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 readonly modifier to internal static members #11777

Merged
1 commit merged into from
May 31, 2020

Conversation

xtqqczze
Copy link
Contributor

@xtqqczze xtqqczze commented Feb 5, 2020

PR Summary

  • Add readonly modifier to internal static members.

PR Context

PR Checklist

@ghost ghost assigned daxian-dbw Feb 5, 2020
@xtqqczze xtqqczze changed the title WIP: Fix S2223 Add readonly modifier to internal static members Feb 5, 2020
@powercode
Copy link
Collaborator

Why readonly instead of const?

@xtqqczze
Copy link
Contributor Author

xtqqczze commented May 11, 2020

@powercode I considered a change to const would have uncertain benefit, and might be a risk, because all variables which consume const would then be hardcoded.

In theory since the fields are internal there should no danger in using const, but I was not confident there could be no issues.

https://blogs.msdn.microsoft.com/armenk/2013/03/01/how-dangerous-can-be-public-const-while-upgrading-or-fixing-a-component/

Armen Kirakosyan's blog
During my investigation I have noticed a source of potential problem for product upgrade where dev team should fix a bug or upgrade a component and distribute it. So what will happen when during fix/upgrade process (I will call it upgrade) a value which was declared as public const was changed. For clear understanding we...

@ghost ghost added the Review - Needed The PR is being reviewed label May 27, 2020
@ghost
Copy link

ghost commented May 27, 2020

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Mainainer, Please provide feedback and/or mark it as Waiting on Author

@xtqqczze
Copy link
Contributor Author

rebased to resolve merge conflicts

@xtqqczze xtqqczze force-pushed the Fix-S2223 branch 2 times, most recently from 1e61728 to 50d31e6 Compare May 27, 2020 15:04
@xtqqczze
Copy link
Contributor Author

@iSazonov I have made amendments, can you review?

@xtqqczze
Copy link
Contributor Author

@iSazonov force pushed to remove name changes, but now we have StyleCop rule violations, SA1304NonPrivateReadonlyFieldsMustBeginWithUpperCaseLetter

@iSazonov iSazonov added the CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log label May 27, 2020
@iSazonov iSazonov added this to the 7.1.0-preview.4 milestone May 27, 2020
@xtqqczze
Copy link
Contributor Author

rebased to resolve merge conflicts

@iSazonov
Copy link
Collaborator

force pushed to remove name changes, but now we have StyleCop rule violations, SA1304NonPrivateReadonlyFieldsMustBeginWithUpperCaseLetter

Our code convention is :

Use _camelCase to name internal and private fields and use readonly where possible.

So we have a conflict with StyleCop rule. But we can not use the rule without changing our code convention.
/cc @daxian-dbw for conclusion.

@ghost ghost removed the Review - Needed The PR is being reviewed label May 29, 2020
@xtqqczze
Copy link
Contributor Author

So we have a conflict with StyleCop rule. But we can not use the rule without changing our code convention.
/cc @daxian-dbw for conclusion.

@iSazonov I have opened #12819 so we can make a general decision on the code convention.

@xtqqczze
Copy link
Contributor Author

@iSazonov I am not sure why CodeFactor finds NonPrivateReadonlyFieldsMustBeginWithUpperCaseLetter issues as the rule is not enabled in setting.stylecop or the default rule-set.

@iSazonov
Copy link
Collaborator

@xtqqczze Perhaps it is CodeFactor effiect.

@iSazonov iSazonov added the AutoMerge informs the bot to automerge the PR label May 31, 2020
@ghost
Copy link

ghost commented May 31, 2020

Hello @iSazonov!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit e93381e into PowerShell:master May 31, 2020
@xtqqczze
Copy link
Contributor Author

@xtqqczze Perhaps it is CodeFactor effiect.

@iSazonov The rule is not present in the CodeFactor default rule-set.

@xtqqczze xtqqczze deleted the Fix-S2223 branch May 31, 2020 15:08
@xtqqczze
Copy link
Contributor Author

@iSazonov I have PR #12855 to disable NonPrivateReadonlyFieldsMustBeginWithUpperCaseLetter in Settings.StyleCop.

@ghost
Copy link

ghost commented Jun 25, 2020

🎉v7.1.0-preview.4 has been released which incorporates this pull request.:tada:

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoMerge informs the bot to automerge the PR CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants