Skip to content
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

Add UseConsistentCasing #1704

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Jaykul
Copy link

@Jaykul Jaykul commented Aug 17, 2021

Add lowercase keyword (and operator) enforcement as a separate rule.

I was thinking about adding a check for the correct case of [Types] and [Attribute()]s, but I think that should probably be in the original rule (UseCorrectCasing), which begs the question of whether we should just add that (or all of this) to the existing rule...

Originally, I made a separate rule because I thought I might need to configure the type of case ("lowercase", "UPPERCASE", "CorrectCase") ... but I quickly decided all-caps is awful, and there is no "correct" case (the C# tokenizer code just capitalizes the first letter of each token, and the documentation is inconsistent about capitalization of keywords and operators).

@Jaykul
Copy link
Author

Jaykul commented Aug 17, 2021

Looks like I missed updating some docs/tests

@Jaykul
Copy link
Author

Jaykul commented Aug 17, 2021

Hold off merging this, because I have realized that it does the wrong thing for a few operators.

@rjmholt rjmholt marked this pull request as draft August 17, 2021 16:43
RuleDocumentation/UseConsistentCasing.md Outdated Show resolved Hide resolved
Rules/Strings.resx Outdated Show resolved Hide resolved
Rules/UseConsistentCasing.cs Outdated Show resolved Hide resolved
Rules/UseConsistentCasing.cs Outdated Show resolved Hide resolved
Rules/UseConsistentCasing.cs Outdated Show resolved Hide resolved
Rules/UseConsistentCasing.cs Outdated Show resolved Hide resolved
Rules/UseConsistentCasing.cs Outdated Show resolved Hide resolved
Rules/UseConsistentCasing.cs Outdated Show resolved Hide resolved
Rules/UseConsistentCasing.cs Outdated Show resolved Hide resolved
Rules/UseConsistentCasing.cs Outdated Show resolved Hide resolved
@Jaykul Jaykul marked this pull request as ready for review August 19, 2021 03:43
Copy link
Collaborator

@bergmeister bergmeister 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 the effort, looks ok from a high level.
But we already have a UseCorrectCasing rule, which is used for cmdlet casing at the moment but I think we could just add CheckOperator and CheckKeyword as options to it and a new option for CheckCmdlet so that user can pick and choose what they prefer. Keeping it in one rule will make things easier to integrate the new settings into the VS-Code extension, which already has scaffolding for UseCorrectCasing. Also, if you wanted the rule to be usable by the formatter and the vs-code extension, you'd have to register it here.
WDYT @rjmholt

@rjmholt
Copy link
Contributor

rjmholt commented Aug 24, 2021

which begs the question of whether we should just add that (or all of this) to the existing rule...

we already have a UseCorrectCasing rule, which is used for cmdlet casing at the moment but I think we could just add CheckOperator and CheckKeyword as options to it and a new option for CheckCmdlet so that user can pick and choose what they prefer

Sorry, I meant to get back to this question earlier.

I think it's better all in one rule, because:

  • We have an ongoing problem in PSScriptAnalyzer where rules that do specific things are given general names and then confusingly have competing rules next to them with a name that makes them hard to differentiate
  • We also have the opposite problem where some rules have very specific names but philosophically should do more (for example AvoidUsingInvokeExpression should also warn against [scriptblock]::Create() and maybe some other things)
  • We have a good story for rule configuration
  • In this particular case, I think the desires to case cmdlets and to case keywords/operators correctly are cohesive and really do belong in one rule
  • We unfortunately also have a bad story for formatting currently, so piggy-backing on an established formatting rule also provides a workaround there

All the code looks good and looks pretty portable at this stage, so hopefully it's just a case of sticking it inside of some conditional methods in UseCorrectCasing, rejigging the configuration and moving the tests over. But let me know if it's more complicated than that.

@Jaykul
Copy link
Author

Jaykul commented Aug 26, 2021

I'm quite happy to merge them. This was a safer PR just because if you didn't want it, I could take my code and go home 😉

I did add this one to the formatter (you can see in the tests that I verified that worked).

@totkeks
Copy link

totkeks commented Nov 8, 2022

Any updates on this PR? Would be nice to have this feature in vscode so it autoformats my keywords properly.

@Jaykul
Copy link
Author

Jaykul commented Nov 9, 2022

I quite forgot about it. I'll try to look at it again this week

@PrzemyslawKlys
Copy link
Contributor

Reminder @Jaykul that it's already next week 😂😂

@bergmeister bergmeister assigned bergmeister and Jaykul and unassigned bergmeister Feb 22, 2023
@ykuijs
Copy link

ykuijs commented Apr 5, 2023

Any update on this PR?

@bergmeister
Copy link
Collaborator

Last chance @Jaykul as we are starting to wrap up for next release this month

@Jaykul
Copy link
Author

Jaykul commented Feb 16, 2024

It's been a long, long time...
There's so much I feel that I should say
But words can wait until some other day

@totkeks
Copy link

totkeks commented Apr 12, 2024

I have seen that this PR has gotten stale a bit again. Is there anything that could be done to help complete it?

I came across this again, because I noticed that I lack an auto-formatter for casing in VS Code for my PowerShell scripts. Which is why it would be really cool to have.

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.

6 participants