Skip to content

Add readonly modifier to internal static members - #11777

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

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

Conversation

@xtqqczze

@xtqqczze xtqqczze commented Feb 5, 2020

Copy link
Copy Markdown
Contributor

PR Summary

  • Add readonly modifier to internal static members.

PR Context

PR Checklist

@xtqqczze xtqqczze changed the title WIP: Fix S2223 Add readonly modifier to internal static members Feb 5, 2020
@powercode

Copy link
Copy Markdown
Collaborator

Why readonly instead of const?

@xtqqczze

xtqqczze commented May 11, 2020

Copy link
Copy Markdown
Contributor Author

Staffan Gustafsson (@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

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

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

Ilya (@iSazonov) I have made amendments, can you review?

Comment thread src/Microsoft.Management.Infrastructure.CimCmdlets/CimSessionOperations.cs Outdated
@xtqqczze

Copy link
Copy Markdown
Contributor Author

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

@iSazonov Ilya (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 Ilya (iSazonov) added this to the 7.1.0-preview.4 milestone May 27, 2020
@xtqqczze

Copy link
Copy Markdown
Contributor Author

rebased to resolve merge conflicts

@iSazonov

Copy link
Copy Markdown
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 Dongbo Wang (@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

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

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

@xtqqczze

Copy link
Copy Markdown
Contributor Author

Ilya (@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

xtqqczze Perhaps it is CodeFactor effiect.

@iSazonov Ilya (iSazonov) added the AutoMerge informs the bot to automerge the PR label May 31, 2020
@ghost

Copy link
Copy Markdown

Hello Ilya (@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 Perhaps it is CodeFactor effiect.

Ilya (@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

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

@ghost

Copy link
Copy Markdown

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

Handy links:

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj/PowerShell that referenced this pull request Aug 6, 2026
# PR Summary

* Add readonly modifier to internal static members.

## PR Context

## PR Checklist

- [x] [PR has a meaningful title](https://github.com/PowerShell/PowerShell/blob/master/.github/CONTRIBUTING.md#pull-request---submission)
    - Use the present tense and imperative mood when describing your changes
- [x] [Summarized changes](https://github.com/PowerShell/PowerShell/blob/master/.github/CONTRIBUTING.md#pull-request---submission)
- [x] [Make sure all `.h`, `.cpp`, `.cs`, `.ps1` and `.psm1` files have the correct copyright header](https://github.com/PowerShell/PowerShell/blob/master/.github/CONTRIBUTING.md#pull-request---submission)
- [x] This PR is ready to merge and is not [Work in Progress](https://github.com/PowerShell/PowerShell/blob/master/.github/CONTRIBUTING.md#pull-request---work-in-progress).
    - If the PR is work in progress, please add the prefix `WIP:` or `[ WIP ]` to the beginning of the title (the `WIP` bot will keep its status check at `Pending` while the prefix is present) and remove the prefix when the PR is ready.
- **[Breaking changes](https://github.com/PowerShell/PowerShell/blob/master/.github/CONTRIBUTING.md#making-breaking-changes)**
    - [x] None
    - **OR**
    - [ ] [Experimental feature(s) needed](https://github.com/MicrosoftDocs/PowerShell-Docs/blob/staging/reference/6/Microsoft.PowerShell.Core/About/about_Experimental_Features.md)
        - [ ] Experimental feature name(s): <!-- Experimental feature name(s) here -->
- **User-facing changes**
    - [x] Not Applicable
    - **OR**
    - [ ] [Documentation needed](https://github.com/PowerShell/PowerShell/blob/master/.github/CONTRIBUTING.md#pull-request---submission)
        - [ ] Issue filed: <!-- Number/link of that issue here -->
- **Testing - New and feature**
    - [x] N/A or can only be tested interactively
    - **OR**
    - [ ] [Make sure you've added a new test if existing tests do not effectively test the code changed](https://github.com/PowerShell/PowerShell/blob/master/.github/CONTRIBUTING.md#before-submitting)
- **Tooling**
    - [x] I have considered the user experience from a tooling perspective and don't believe tooling will be impacted.
    - **OR**
    - [ ] I have considered the user experience from a tooling perspective and enumerated concerns in the summary. This may include:
        - Impact on [PowerShell Editor Services](https://github.com/PowerShell/PowerShellEditorServices) which is used in the [PowerShell extension](https://github.com/PowerShell/vscode-powershell) for VSCode (which runs in a different PS Host).
        - Impact on Completions (both in the console and in editors) - one of PowerShell's most powerful features.
        - Impact on [PSScriptAnalyzer](https://github.com/PowerShell/PSScriptAnalyzer) (which provides linting & formatting in the editor extensions).
        - Impact on [EditorSyntax](https://github.com/PowerShell/EditorSyntax) (which provides syntax highlighting with in VSCode, GitHub, and many other editors).
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.

4 participants