X Tutup
Skip to content

Add readonly modifier to internal static members#11777

Merged
1 commit merged intoPowerShell:masterfrom
xtqqczze:Fix-S2223
May 31, 2020
Merged

Add readonly modifier to internal static members#11777
1 commit merged intoPowerShell:masterfrom
xtqqczze:Fix-S2223

Conversation

@xtqqczze
Copy link
Copy Markdown
Contributor

@xtqqczze xtqqczze commented Feb 5, 2020 •
edited
Loading

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
Copy Markdown
Collaborator

powercode commented May 10, 2020

Why readonly instead of const?

@xtqqczze
Copy link
Copy Markdown
Contributor Author

xtqqczze commented May 11, 2020 •
edited by unfurl-links bot
Loading

@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
Copy Markdown

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
Copy Markdown
Contributor Author

xtqqczze commented May 27, 2020

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
Copy Markdown
Contributor Author

xtqqczze commented May 27, 2020

@iSazonov I have made amendments, can you review?

@xtqqczze
Copy link
Copy Markdown
Contributor Author

xtqqczze commented May 27, 2020

@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 xtqqczze mentioned this pull request May 27, 2020
Closed
@xtqqczze
Copy link
Copy Markdown
Contributor Author

xtqqczze commented May 29, 2020

rebased to resolve merge conflicts

@iSazonov
Copy link
Copy Markdown
Collaborator

iSazonov commented May 29, 2020

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
Copy Markdown
Contributor Author

xtqqczze commented May 29, 2020

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
Copy Markdown
Contributor Author

xtqqczze commented May 29, 2020

@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
Copy Markdown
Collaborator

iSazonov commented May 31, 2020

@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
Copy Markdown

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
Copy Markdown
Contributor Author

xtqqczze commented May 31, 2020

@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
Copy Markdown
Contributor Author

xtqqczze commented May 31, 2020

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

@ghost
Copy link
Copy Markdown

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

Reviewers

@iSazonov iSazonov iSazonov approved these changes

@adityapatwardhan adityapatwardhan Awaiting requested review from adityapatwardhan adityapatwardhan is a code owner

@anmenaga anmenaga Awaiting requested review from anmenaga

@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw daxian-dbw is a code owner

Assignees

@daxian-dbw daxian-dbw

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

Milestone

7.1.0-preview.4

Development

Successfully merging this pull request may close these issues.

4 participants

@xtqqczze @powercode @iSazonov @daxian-dbw
X Tutup