-
Notifications
You must be signed in to change notification settings - Fork 234
1.x: Fix wiring of WhitespaceInsideBrace and WhitespaceAroundPipe that got broken in #1024 #1142
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
1.x: Fix wiring of WhitespaceInsideBrace and WhitespaceAroundPipe that got broken in #1024 #1142
Conversation
{ "CheckOperator", WhitespaceAroundOperator }, | ||
{ "CheckSeparator", WhitespaceAfterSeparator }, | ||
{ "CheckInnerBrace", WhitespaceInsideBrace }, | ||
{ "CheckPipe", WhitespaceAroundPipe }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line and the line above is the essential fix
"PSUseConsistentWhitespace", | ||
"PSUseConsistentIndentation", | ||
"PSAlignAssignmentStatement", | ||
"PSAvoidUsingCmdletAliases", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This entry (the removed PSAvoidUsingCmdletAliases
) is actually not necessary and was removed in PR #1056 in master
, hence why this change is also brought back as well as the essential required thing is ruleConfigurations.Add("PSAvoidUsingCmdletAliases", new Hashtable());
a few lines above here for the autoCorrectAliases
feature to work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that we want to keep changes to a minimum in the 1.x branch, would you be able to take out all the formatting changes?
@rjmholt I could but the whole point of bringing the formatting changes from master into the 1.x is branch that cherry-picking would work again without silly white-space merge conflicts that could lead to cause mistakes like this. Do you still not agree? |
If that's the original cause of the issue then that's a fair point. I'll let @TylerLeonhardt decide there. There shouldn't be any more cherry picks though. This PR is probably the only change we'll make before a final release of the 1.x extension. Later this month the preview branch will become stable and preview/stable will behave like they were originally intended to. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Yeah I'm fine with that. I just reviewed the whole thing line-by-line. Looks good. |
@rjmholt can you review as well though? |
Fixes PowerShell/vscode-powershell#2375
More settings got broken in this bad cherry pick, I can only assume that there was a merge conflict that was resolved incorrectly.
To make it simpler in the future, I copied the method from master (hence the whitespace diff)
cc @TylerLeonhardt