Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upFix PlaceOpenBrace rule correction to take comment at the end of line into account #929
Conversation
|
Just a couple comments. Otherwise, LGTM! Although I should not be the only sign off on this. |
| corrections.Add( | ||
| new CorrectionExtent( | ||
| precedingExpression.Extent.StartLineNumber, | ||
| lCurly.Extent.EndLineNumber, | ||
| precedingExpression.Extent.StartColumnNumber, | ||
| lCurly.Extent.EndColumnNumber, | ||
| precedingExpression.Text + " " + lCurly.Text, | ||
| $"{precedingExpression.Text} {lCurly.Text}{optionalComment}", |
This comment has been minimized.
This comment has been minimized.
TylerLeonhardt
Mar 20, 2018
Member
without knowing too much about this project... should there be a space here between the }{:
{lCurly.Text}{optionalComment} for when optionalComment isn't empty string and actually is a comment?
This comment has been minimized.
This comment has been minimized.
bergmeister
Mar 20, 2018
Author
Collaborator
That is the point of having the separate optionalComment variable, if it is not an empty string, then the variable itself contains the preceding space character. This way I avoid appending a space character when there is no comment.
| '@ | ||
| Invoke-Formatter -ScriptDefinition $scriptDefinition -Settings $settings | Should -Be $expected | ||
| } | ||
|
|
This comment has been minimized.
This comment has been minimized.
TylerLeonhardt
Mar 20, 2018
Member
Is it possible to have the reverse of this?
We have:
foreach ($x in $y) { # PSSA is awesome
Get-Something
}
and want:
foreach ($x in $y) # PSSA is awesome
{
Get-Something
}
would it be worth having a test for that?
This comment has been minimized.
This comment has been minimized.
bergmeister
Mar 20, 2018
Author
Collaborator
This is a very good point. We should test this case as well, I have not thought of that.
This comment has been minimized.
This comment has been minimized.
bergmeister
Mar 23, 2018
Author
Collaborator
I added a test case for this now and added implicit integration tests with the code formatting style presets that VSCode uses.
|
lgtm |
…n the next line to a brace style where the brace is on the same line as suggested in the PR. Added more assertions for an integration test with code formatting styles
…ext block (they screwed up following tests for some reason).
bergmeister commentedMar 11, 2018
•
edited
PR Summary
Fixes #826 which came indirectly via 2 raised VSCode issues.
Fixes #800
Basically when a comment was at the end of the line, then if the rule had settings to move the brace on the same line, it moved it at the end of the line AFTER the comment.
This was because the correction just took the last token. This PR checks if the last token is a comment and if so takes the second last token if possible and appends the comment after the opening brace (including a preceding space character).
Other than that some minor tidy up.
I have locally tested that this also works in PSCore.
PR Checklist
Note: Tick the boxes below that apply to this pull request by putting an
xbetween the square brackets. Please mark anything not applicable to this PRNA.WIP:to the beginning of the title and remove the prefix when the PR is ready