Add option to customize number of spaces leading // comments
#776
Conversation
| // Number of spaces that preceeds line comments. | ||
| public var spacesBeforeLineComments: Int |
There was a problem hiding this comment.
Does this apply to both // and /**/ comments? The test case with /**/ has 1-space in both the input and expected, so wasn't clear from there.
There was a problem hiding this comment.
I believe /* */ is referred to as "block comments" regardless of whether it's on the same line or not. In existing unit tests (adjacent to the ones I added), there are /* */ comments with 1 leading space that aren't affected by the formatting (which ensures 2 spaces). I made sure in the documentation, it explicitly says "comments starting with '//'" to avoid this ambiguity.
There was a problem hiding this comment.
Ah I see, thanks. Could you make the comment here a doc comment (///) and include that here as well? @allevato is it intended that block comments are treated differently in this case? Not sure I have much of an opinion on it either way.
There was a problem hiding this comment.
The name of this configuration should be explicit that it's just about "end of line comments", since that traditionally means // comments that specifically follow something else on a line and does not include /* ... */, ///, or /** ... */ comments, nor // comments on their own line. So I'd call this spacesBeforeEndOfLineComments.
I think it's correct to only support this for // EoL comments. Let's imagine we have a situation where we want 2 spaces before those, but only one space around inline block comments, and we have this contrived example:
let x = 5 // something
let y = someValue + x /* meters */ / someOtherValue /* seconds */When we add another space before the comment on the first line, I don't think it would be right to change the spacing around the block comments on the second line.
| @@ -199,6 +200,127 @@ final class CommentTests: PrettyPrintTestCase { | |||
| assertPrettyPrintEqual(input: input, expected: expected, linelength: 45) | |||
| } | |||
|
|
|||
| func testLineCommentsWithCustomLeadingSpaces() { | |||
There was a problem hiding this comment.
I think it would help to split this test case up - it's quite hard to compare the input/expectation at the moment.
There was a problem hiding this comment.
it's a copy of the previous tests. I can try and split of both?
There was a problem hiding this comment.
I'm happy to just have the new tests split, feel free to also clean up the copied-from in another PR though.
There was a problem hiding this comment.
Personally I'd split them into functions so the failure is super obvious if it happens. Currently you'd have to map the failed input/output back to one of the pairs by looking over them all (since the failure would always be on the same line). You could also add a line number to each test case, but at that point having separate functions seems easier.
Any preference @allevato? I'm also happy to take as is, it's just tests.
Add an option `spacesBeforeLineComments` that allows customization of number of spaces between the first non-trivial token and a line comment with the `//` prefix. Defaults to 2, the historical default value.
dduan
force-pushed
the
dd/line-comment-leading-spaces
branch
from
b634fd3
to
4a99f1c
Compare
| @@ -199,6 +200,127 @@ final class CommentTests: PrettyPrintTestCase { | |||
| assertPrettyPrintEqual(input: input, expected: expected, linelength: 45) | |||
| } | |||
|
|
|||
| func testLineCommentsWithCustomLeadingSpaces() { | |||
There was a problem hiding this comment.
Personally I'd split them into functions so the failure is super obvious if it happens. Currently you'd have to map the failed input/output back to one of the pairs by looking over them all (since the failure would always be on the same line). You could also add a line number to each test case, but at that point having separate functions seems easier.
Any preference @allevato? I'm also happy to take as is, it's just tests.
Co-authored-by: Ben Barham <b.n.barham@gmail.com>
|
@allevato thoughts? |
|
Thanks @allevato! FWIW, I personally don't hold any opinions about any of the options. But getting swift-format to adoption for my team requires some finessing such as this :) |
|
Does anyone know when this version of |
|
This change will be included in the swift-format bundled with Swift 6.1. |
Add an option
spacesBeforeEndOfLineCommentsthat allows customization of number of spaces between the first non-trivial token and a line comment with the//prefix. Defaults to 2, the historical default value.