Skip to content

Merge adjacent .line and .docLine comments into a single element. #732

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 9, 2024

Conversation

shawnhyam
Copy link
Contributor

There is existing code to do this (for .docLine comments) but it isn't triggering due to the soft breaks in the token stream between every comment. This is a necessary first step for formatting line comments.

Copy link
Member

@allevato allevato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this generally as a way to move us toward being able to reflow comments as a unit (I saw your earlier draft PR). In addition to the comments below, can you please add some more tests? Comments are probably an area where we're a little bit weaker on test coverage, and it would be good to make sure we don't break anything obvious with this change. You don't have to test that the comments themselves got merged in the data model (since you don't have direct access to the token stream outside of the pretty printer), but tests verifying that the final indentation and line breaks are what you expect would be fine.

return
case (.break(.same, _, .soft(let count, _)), .comment(let c2, _))
where count == 1 && (c2.kind == .docLine || c2.kind == .line):
// we are search for the pattern of [line comment] - [soft break 1] - [line comment]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we limit ourselves to just .same type breaks? Here's a weird example:

for x: Int
  // foo
  // bar
in y {}

This produces the following token stream around the comments:

  [BREAK Kind: continue Size: 1 Length: 107 NL: soft(count: 1, discretionary: true) Idx: 10]
  [COMMENT Line Length: 7 EOL: false Idx: 11]
  // foo
  [BREAK Kind: continue Size: 0 Length: 107 NL: soft(count: 1, discretionary: true) Idx: 12]
  [COMMENT Line Length: 7 EOL: false Idx: 13]
  // bar
  [BREAK Kind: continue Size: 0 Length: 104 NL: soft(count: 1, discretionary: true) Idx: 14]

This uses .continue breaks, so the logic as written here wouldn't merge those, even though visually they look like a multiline comment.

We'd also want to distinguish that from a similar case:

for x: Int  // foo
  // bar
in y {}

Presumably we wouldn't want to merge those comments together. I think the logic you have here is fine for that one, because the token stream looks like this:

  [SPACE Size: 2 Flexible: true Length: 2 Idx: 9]
  [COMMENT Line Length: 7 EOL: true Idx: 10]
  // foo
  [BREAK Kind: close(mustBreak: false) Size: 0 Length: 0 NL: elective(ignoresDiscretionary: false) Idx: 11]
  [BREAK Kind: continue Size: 1 Length: 107 NL: soft(count: 1, discretionary: true) Idx: 12]
  [COMMENT Line Length: 7 EOL: false Idx: 13]
  // bar
  [BREAK Kind: continue Size: 0 Length: 104 NL: soft(count: 1, discretionary: true) Idx: 14]

There are two breaks after foo (the first closes the indenting break group around the type annotation, the second is for the newline), so this logic (correctly) wouldn't trigger.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it to allow merging of .continue and .contextual breaks, based on your feedback and examining the token stream from an existing codebase.

Copy link
Member

@allevato allevato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this!

@allevato allevato merged commit 4f19acc into swiftlang:main May 9, 2024
@shawnhyam shawnhyam deleted the collect-line-comments branch May 29, 2024 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants