Skip to content

OBSOLETE: Update accessibility modifiers - #11773

Closed
xtqqczze wants to merge 8 commits into
PowerShell:masterfrom
xtqqczze:static-classes
Closed

xtqqczze wants to merge 8 commits into
PowerShell:masterfrom
xtqqczze:static-classes

Conversation

@xtqqczze

@xtqqczze xtqqczze commented Feb 5, 2020 •

Copy link
Copy Markdown
Contributor

PR Summary

  • Autofix RCS1102: Make class static when only containing static members.
    • plus 4 manual fixes
  • Autofix RCS1225: Make class sealed when only having a private constructor.
    • Fix CA1047: Do not declare protected members in sealed types
  • Change sealed classes with private constructors to static classes
  • Revert changes to public APIs causing test failures

PR Context

https://github.com/PowerShell/PowerShell/blob/master/docs/dev-process/coding-guidelines.md#member-conventions

PR Checklist

@vexx32

Copy link
Copy Markdown
Collaborator

xtqqczze you may want to update the PR description with your reasoning for each change, since most of them seem to be on a per-class basis. I'd also recommend a more general PR title, since it seems you're not only adding static modifiers here, so it's probably a little less confusing to use something like Update class modifiers and explain static / sealed / etc for each class you're changing in the PR description. 🙂

@xtqqczze xtqqczze changed the title WIP: Make classes having only static members static WIP: Change class modifiers to static where applicable Feb 5, 2020
@iSazonov Ilya (iSazonov) added the CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log label Feb 5, 2020
@iSazonov Ilya (iSazonov) added this to the 7.1.0-preview.1 milestone Feb 5, 2020

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The class contains only static members.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ilya (@iSazonov) Should I revert?

@xtqqczze xtqqczze changed the title WIP: Change class modifiers to static where applicable WIP: Update accessibility modifiers Feb 5, 2020
@xtqqczze
xtqqczze force-pushed the static-classes branch 2 times, most recently from c7891dd to 0c99e14 Compare February 5, 2020 06:21

@iSazonov Ilya (iSazonov) left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we set sealed for internal classes? Can you add more info in the PR description?

@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

@iSazonov

Copy link
Copy Markdown
Collaborator

xtqqczze Have you plans to continue? Please resolve merge conflicts.

@ghost ghost removed the Review - Needed The PR is being reviewed label Oct 25, 2020
@xtqqczze
xtqqczze marked this pull request as draft October 26, 2020 16:23
@xtqqczze

Copy link
Copy Markdown
Contributor Author

I will separate changes into new PRs, converting to draft.

@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Oct 29, 2020
@iSazonov

Copy link
Copy Markdown
Collaborator

I will separate changes into new PRs, converting to draft.

Have you any updates?

@xtqqczze

xtqqczze commented Nov 5, 2020

Copy link
Copy Markdown
Contributor Author

I will separate changes into new PRs, converting to draft.

Have you any updates?

@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Nov 5, 2020
@iSazonov

Copy link
Copy Markdown
Collaborator

I will separate changes into new PRs, converting to draft.

Have you any updates?

So the PR can be closed?

@xtqqczze xtqqczze changed the title Update accessibility modifiers WIP: Update accessibility modifiers Nov 5, 2020
@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Nov 9, 2020
@iSazonov

Copy link
Copy Markdown
Collaborator

I will separate changes into new PRs, converting to draft.

Have you any updates?

So the PR can be closed?

xtqqczze Can we close?

@xtqqczze

Copy link
Copy Markdown
Contributor Author

xtqqczze Can we close?

This PR has some changes that depends on #13897, so it should stay as draft.

@ghost ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Nov 23, 2020
@xtqqczze

xtqqczze commented Dec 6, 2020

Copy link
Copy Markdown
Contributor Author

RCS1102 changes are split to #14335.

@ghost ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Dec 6, 2020
@ghost ghost added the Stale label Dec 24, 2020
@ghost

Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 15 days. It will be closed if no further activity occurs within 10 days of this comment.

@xtqqczze

Copy link
Copy Markdown
Contributor Author

e0e60ba changes submitted as #15671.

@ghost ghost removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept Stale labels Jun 25, 2021
@xtqqczze
xtqqczze deleted the static-classes branch June 25, 2021 14:09
@xtqqczze xtqqczze mentioned this pull request Jun 25, 2021
@xtqqczze

Copy link
Copy Markdown
Contributor Author

2069d92 changes submitted as #15675

@xtqqczze xtqqczze changed the title WIP: Update accessibility modifiers OBSOLETE: Update accessibility modifiers Jun 25, 2021
@xtqqczze

Copy link
Copy Markdown
Contributor Author

Changes from this PR are now either merged or submitted in PRs.

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

Reformat using `csharp_preferred_modifier_order`.

## PR Context

Split change from PowerShell#11773

## 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

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.

6 participants