Formatter: Show preceding, following and enclosing nodes of comments, Attempt 2 #6813

konstin · 2023-08-23T11:54:15Z

This is a new attempt after #6337.

Summary I used to always add dbg! for the preceding, following and enclosing node. With this change --print-comments can do this instead.

match foo: # a
    # b
    case [1, 2, * # c
    rest]:
        pass
    # d
    case [1, 2, * #e
    _]:
        pass
    case [
        1,
        2,
        *rest,
    ]:
        pass

Additional --print-comments Output:

11..14 Some((ExprName, 6..9)) Some((MatchCase, 27..68)) (StmtMatch, 0..186) "# a"
19..22 Some((ExprName, 6..9)) Some((MatchCase, 27..68)) (StmtMatch, 0..186) "# b"
41..44 None None (PatternMatchStar, 39..53) "# c"
73..76 Some((MatchCase, 27..68)) Some((MatchCase, 81..118)) (StmtMatch, 0..186) "# d"
95..97 None None (PatternMatchStar, 93..103) "#e"

Test Plan n/a

konstin · 2023-08-23T11:54:52Z

crates/ruff_python_formatter/src/comments/visitor.rs

-    fn add_comment(&mut self, placement: CommentPlacement<'a>) {
+impl<'a> CommentsBuilderTrait<'a> for CommentsBuilder<'a> {
+    fn add_comment(&mut self, comment: DecoratedComment<'a>, locator: &Locator) {
+        let placement = place_comment(comment, locator);


i didn't find a better location for this

konstin · 2023-08-23T11:55:20Z

crates/ruff_python_formatter/src/comments/visitor.rs

-pub(crate) struct CommentsVisitor<'a> {
-    builder: CommentsBuilder<'a>,
+struct CommentsVisitor<'a, 'builder> {
+    builder: &'builder mut (dyn CommentsBuilderTrait<'a> + 'a),


side effect: the visitor is now private

MichaReiser · 2023-08-23T12:45:07Z

Can you run some local benchmarks to confirm/deny the performance regression?

MichaReiser · 2023-08-26T16:07:15Z

Should we just use debug! or something similar to print out the comments during debugging? We could even gate it with cfg(debug_assertions) to make sure it doesn't make it into production builds.

konstin · 2023-08-26T16:08:48Z

you mean through tracing?

codspeed-hq · 2023-08-26T16:12:08Z

CodSpeed Performance Report

Merging #6813 will not alter performance

_{Comparing collect_decorated_comments2 (b153208) with main (15b73bd)}

Summary

✅ 16 untouched benchmarks

MichaReiser · 2023-09-06T07:38:50Z

crates/ruff_python_formatter/src/comments/mod.rs

+    /// Extracts the comments from the AST.
+    pub(crate) fn collect_decorated_comments(
+        root: &'a Mod,
+        source_code: SourceCode<'a>,
+        comment_ranges: &'a CommentRanges,
+    ) -> Vec<DecoratedComment<'a>> {
+        collect_comments(root, source_code, comment_ranges)
+    }
+


This method feels misplaced because it doesn't involve Comments at all. Why not expose collect_comments directly?

MichaReiser · 2023-09-06T07:41:32Z

crates/ruff_python_formatter/src/comments/visitor.rs

+    let mut builder = CommentsBuilder::default();
+    CommentsVisitor::new(source_code, comment_ranges, &mut builder).visit(root);
+    builder.finish()


Nit: I'm leaning towards inlining this function into Comments::from_ast because get_comments_map sounds like your getting a value rather than computing it. Maybe rename it to build_comments_map? IMO, CommentsVisitor and CommentsBuilder being visible to the comments module seems reasonable to me.

MichaReiser · 2023-09-06T07:48:59Z

crates/ruff_python_formatter/src/comments/visitor.rs

@@ -512,13 +529,29 @@ impl<'a> CommentPlacement<'a> {
    }
 }

+pub(super) trait CommentsBuilderTrait<'a> {


It's uncommon to include the type-kind in the name, especially because only the CommentsBuilder is building Comments.

A rusty name could be AddComment, or we use Add<DecoratedComment> and make locator a field on CommentsBuilder.

Or another idea is to accept any type that implements Extend. The downside is that we'll have to pass [comment] because extend_one isn't stable. But I think that resembles the idea the closest. You get a comment and can append it to some data structure. Which also removes the need for CommentsCollector because you can simply pass a &mut Vec.

I think I'm then leaning to making collect_comments public and make it accept any &dyn Extend<DecoratedComment<'a>> and to make CommentsBuilder pub(super)`.

I went with PushComment::push_comment, mirroring Vec. A possible improvement could be implementing PushComment directly on CommentsMap<'a> and Vec<DecoratedComment<'a>>. I think we do need a dedicated trait though.

I don't mind implementing it directly on Vec<DecoratedComment> but I think I would keep the builder around since we don't want adding the Locator to CommentsMap.

… Attempt 2

MichaReiser · 2023-09-06T10:55:52Z

One thing I forgot to mention before. I struggle reading the output of the new comment because it isn't clear to me which node is the enclosing, trailing, or leading node. I think it would help to have labels and/or use Rust's expanded struct display

…, Attempt 2 (#6813)

**Summary** Show a header in the formatter comment decoration debug output that shows which node is preceding/following/enclosing (#6813 (comment)). I also kept this intentionally condensed to make it easy to use this is a small sidebar without vertical scrolling. ``` # Comment decoration: Range Preceding Following Enclosing Comment 1292..1298 Some((ExprName, 1274..1280)) Some((ExprName, 1307..1310)) (StmtAnnAssign, 1274..1316) "# type" 2507..2555 None Some((StmtIf, 2634..2891)) (StmtClassDef, 2478..3279) "# didn't match the name in the C implementation," 2560..2629 None Some((StmtIf, 2634..2891)) (StmtClassDef, 2478..3279) "# meaning it is only *safe* to pass it as a keyword argument on 3.12+" 3281..3298 Some((StmtClassDef, 2478..3279)) Some((StmtFunctionDef, 3300..3317)) (ModModule, 0..4162) "# Top level rules" 3481..3496 Some((StmtClassDef, 3452..3479)) Some((StmtClassDef, 3498..3876)) (ModModule, 0..4162) "# Nesting rules" { Node { kind: ExprName, range: 1307..1310, source: `int`, }: { ... ``` **Test Plan** It's debug output.

**Summary** The comment visitor used to rebuild the locator for every comment. Instead, we now keep the locator on the builder. Follow-up to #6813. **Test Plan** No formatting changes.

**Summary** Show a header in the formatter comment decoration debug output that shows which node is preceding/following/enclosing (#6813 (comment)). I also kept this intentionally condensed to make it easy to use this is a small sidebar without vertical scrolling. ``` # Comment decoration: Range Preceding Following Enclosing Comment 1292..1298 Some((ExprName, 1274..1280)) Some((ExprName, 1307..1310)) (StmtAnnAssign, 1274..1316) "# type" 2507..2555 None Some((StmtIf, 2634..2891)) (StmtClassDef, 2478..3279) "# didn't match the name in the C implementation," 2560..2629 None Some((StmtIf, 2634..2891)) (StmtClassDef, 2478..3279) "# meaning it is only *safe* to pass it as a keyword argument on 3.12+" 3281..3298 Some((StmtClassDef, 2478..3279)) Some((StmtFunctionDef, 3300..3317)) (ModModule, 0..4162) "# Top level rules" 3481..3496 Some((StmtClassDef, 3452..3479)) Some((StmtClassDef, 3498..3876)) (ModModule, 0..4162) "# Nesting rules" { Node { kind: ExprName, range: 1307..1310, source: `int`, }: { ... ``` **Test Plan** It's debug output.

Show header for formatter comment decoration info **Summary** Show a header in the formatter comment decoration debug output that shows which node is preceding/following/enclosing (#6813 (comment)). I kept this intentionally condensed to make it easy to use this is a small sidebar without vertical scrolling. ```console $ cargo run --bin ruff_python_formatter -- --emit stdout --print-comments scratch.py # Comment decoration: Range, Preceding, Following, Enclosing, Comment 17..20, Some((ParameterWithDefault, 6..10)), None, (Parameters, 5..22), "# a" 44..47, Some((StmtExpr, 28..39)), Some((StmtExpr, 52..60)), (StmtFunctionDef, 0..60), "# b" 77..80, None, None, (ExprList, 71..82), "# c" { Node { kind: ParameterWithDefault, range: 6..10, source: `x=[]`, }: { ... ``` **Test Plan** It's debug output.

konstin added the internal An internal refactor or improvement label Aug 23, 2023

konstin requested a review from MichaReiser August 23, 2023 11:54

konstin commented Aug 23, 2023

View reviewed changes

konstin mentioned this pull request Aug 23, 2023

Formatter: Show preceding, following and enclosing nodes of comments #6337

Closed

This comment was marked as outdated.

Sign in to view

konstin force-pushed the collect_decorated_comments2 branch from 909e1af to b153208 Compare August 26, 2023 15:58

MichaReiser approved these changes Sep 6, 2023

View reviewed changes

konstin force-pushed the collect_decorated_comments2 branch 2 times, most recently from 35e6f9a to c7e658e Compare September 6, 2023 09:23

Formatter: Show preceding, following and enclosing nodes of comments,…

527aa84

… Attempt 2

konstin force-pushed the collect_decorated_comments2 branch from c7e658e to 527aa84 Compare September 6, 2023 09:27

konstin merged commit 447b7cb into main Sep 6, 2023
16 checks passed

konstin deleted the collect_decorated_comments2 branch September 6, 2023 10:26

zanieb mentioned this pull request Sep 6, 2023

Add warnings for nursery and preview rule selection #7210

Merged

zanieb pushed a commit that referenced this pull request Sep 6, 2023

Formatter: Show preceding, following and enclosing nodes of comments…

55bba99

…, Attempt 2 (#6813)

konstin mentioned this pull request Sep 7, 2023

Reuse locator in formatter comments #7227

Merged

konstin mentioned this pull request Sep 7, 2023

Show header for formatter comment decoration info #7228

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Formatter: Show preceding, following and enclosing nodes of comments, Attempt 2 #6813

Formatter: Show preceding, following and enclosing nodes of comments, Attempt 2 #6813

konstin commented Aug 23, 2023

konstin Aug 23, 2023

konstin Aug 23, 2023 •

edited

Loading

This comment was marked as outdated.

MichaReiser commented Aug 23, 2023

MichaReiser commented Aug 26, 2023 •

edited

Loading

konstin commented Aug 26, 2023

codspeed-hq bot commented Aug 26, 2023

MichaReiser Sep 6, 2023

MichaReiser Sep 6, 2023

MichaReiser Sep 6, 2023

konstin Sep 6, 2023

MichaReiser Sep 6, 2023

MichaReiser commented Sep 6, 2023

Formatter: Show preceding, following and enclosing nodes of comments, Attempt 2 #6813

Formatter: Show preceding, following and enclosing nodes of comments, Attempt 2 #6813

Conversation

konstin commented Aug 23, 2023

konstin Aug 23, 2023

Choose a reason for hiding this comment

konstin Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

This comment was marked as outdated.

MichaReiser commented Aug 23, 2023

MichaReiser commented Aug 26, 2023 • edited Loading

konstin commented Aug 26, 2023

codspeed-hq bot commented Aug 26, 2023

CodSpeed Performance Report

Merging #6813 will not alter performance

Summary

MichaReiser Sep 6, 2023

Choose a reason for hiding this comment

MichaReiser Sep 6, 2023

Choose a reason for hiding this comment

MichaReiser Sep 6, 2023

Choose a reason for hiding this comment

konstin Sep 6, 2023

Choose a reason for hiding this comment

MichaReiser Sep 6, 2023

Choose a reason for hiding this comment

MichaReiser commented Sep 6, 2023

konstin Aug 23, 2023 •

edited

Loading

MichaReiser commented Aug 26, 2023 •

edited

Loading