OBSOLETE: Update accessibility modifiers by xtqqczze · Pull Request #11773 · PowerShell/PowerShell

xtqqczze · 2020-02-05T01:21:12Z

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

PR has a meaningful title
- Use the present tense and imperative mood when describing your changes
Summarized changes
Make sure all .h, .cpp, .cs, .ps1 and .psm1 files have the correct copyright header
This PR is ready to merge and is not 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
- None
- OR
- [NA] Experimental feature(s) needed
  - [NA] Experimental feature name(s):
User-facing changes
- Not Applicable
- OR
- [NA] Documentation needed
  - [NA] Issue filed:
Testing - New and feature
- N/A or can only be tested interactively
- OR
- [NA] Make sure you've added a new test if existing tests do not effectively test the code changed
Tooling
- I have considered the user experience from a tooling perspective and don't believe tooling will be impacted.
- OR
- [NA] I have considered the user experience from a tooling perspective and enumerated concerns in the summary. This may include:
  - Impact on PowerShell Editor Services which is used in the PowerShell extension 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 (which provides linting & formatting in the editor extensions).
  - Impact on EditorSyntax (which provides syntax highlighting with in VSCode, GitHub, and many other editors).

vexx32 · 2020-02-05T01:28:15Z

@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. 🙂

src/System.Management.Automation/CoreCLR/CorePsAssemblyLoadContext.cs

src/Microsoft.Management.UI.Internal/ShowCommand/ViewModel/CommandViewModel.cs

iSazonov · 2020-02-05T04:17:51Z

src/System.Management.Automation/engine/runtime/CompiledScriptBlock.cs

@PaulHigin @TravisEz13 Can we make the class static?

The class contains only static members.

@iSazonov Should I revert?

src/System.Management.Automation/engine/NullString.cs

src/System.Management.Automation/engine/InvocationInfo.cs

src/System.Management.Automation/engine/MshObject.cs

src/System.Management.Automation/engine/ProxyCommand.cs

iSazonov

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

ghost · 2020-05-27T02:01:53Z

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 · 2020-05-27T07:01:43Z

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

TravisEz13 · 2020-10-25T16:22:42Z

Please separate the static and sealed changes.

xtqqczze · 2020-10-26T16:23:54Z

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

iSazonov · 2020-11-05T05:55:37Z

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

Have you any updates?

xtqqczze · 2020-11-05T16:16:58Z

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

Have you any updates?

iSazonov · 2020-11-05T18:09:39Z

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

Have you any updates?

WIP: Enable CA1822: Mark private members as static #13897

Merged: Mark PowerShellAssemblyLoadContextInitializer with static modifier #13874

So the PR can be closed?

iSazonov · 2020-11-21T15:36:27Z

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

Have you any updates?

WIP: Enable CA1822: Mark private members as static #13897

Merged: Mark PowerShellAssemblyLoadContextInitializer with static modifier #13874

So the PR can be closed?

@xtqqczze Can we close?

xtqqczze · 2020-11-23T02:27:18Z

@xtqqczze Can we close?

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

xtqqczze · 2020-12-06T16:55:31Z

RCS1102 changes are split to #14335.

ghost · 2020-12-24T20:00:09Z

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 · 2021-06-25T13:30:21Z

e0e60ba changes submitted as #15671.

xtqqczze · 2021-06-25T14:14:40Z

2069d92 changes submitted as #15675

xtqqczze · 2021-06-25T14:15:54Z

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

xtqqczze requested review from adityapatwardhan and daxian-dbw as code owners February 5, 2020 01:21

ghost assigned iSazonov Feb 5, 2020

xtqqczze force-pushed the static-classes branch from a21bc7b to 7a900fb Compare February 5, 2020 02:34

xtqqczze requested review from TravisEz13 and anmenaga as code owners February 5, 2020 02:34

xtqqczze changed the title ~~WIP: Make classes having only static members static~~ WIP: Change class modifiers to static where applicable Feb 5, 2020

iSazonov reviewed Feb 5, 2020

View reviewed changes

src/System.Management.Automation/CoreCLR/CorePsAssemblyLoadContext.cs Outdated Show resolved Hide resolved

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 added this to the 7.1.0-preview.1 milestone Feb 5, 2020

iSazonov reviewed Feb 5, 2020

View reviewed changes

src/Microsoft.Management.UI.Internal/ShowCommand/ViewModel/CommandViewModel.cs Outdated Show resolved Hide resolved

iSazonov reviewed Feb 5, 2020

View reviewed changes

xtqqczze changed the title ~~WIP: Change class modifiers to static where applicable~~ WIP: Update accessibility modifiers Feb 5, 2020

xtqqczze commented Feb 5, 2020

View reviewed changes

src/System.Management.Automation/engine/NullString.cs Outdated Show resolved Hide resolved

xtqqczze commented Feb 5, 2020

View reviewed changes

src/System.Management.Automation/engine/InvocationInfo.cs Outdated Show resolved Hide resolved

xtqqczze commented Feb 5, 2020

View reviewed changes

src/System.Management.Automation/engine/MshObject.cs Outdated Show resolved Hide resolved

xtqqczze commented Feb 5, 2020

View reviewed changes

src/System.Management.Automation/engine/ProxyCommand.cs Outdated Show resolved Hide resolved

xtqqczze force-pushed the static-classes branch 2 times, most recently from c7891dd to 0c99e14 Compare February 5, 2020 06:21

iSazonov reviewed Feb 5, 2020

View reviewed changes

daxian-dbw modified the milestones: 7.1.0-preview.1, 7.1.0-preview.2 Apr 1, 2020

TravisEz13 modified the milestones: 7.1.0-preview.2, 7.1.0-preview.3 Apr 24, 2020

TravisEz13 modified the milestones: 7.1.0-preview.3, 7.1.0-preview.4 May 14, 2020

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

ghost removed the Review - Needed The PR is being reviewed label Oct 25, 2020

xtqqczze marked this pull request as draft October 26, 2020 16:23

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

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

xtqqczze changed the title ~~Update accessibility modifiers~~ WIP: Update accessibility modifiers Nov 5, 2020

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

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

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 added the Stale label Dec 24, 2020

ghost closed this Jan 4, 2021

iSazonov mentioned this pull request Jun 24, 2021

Improve performance by using sealed classes #15110

Closed

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 deleted the static-classes branch June 25, 2021 14:09

xtqqczze mentioned this pull request Jun 25, 2021

Make classes sealed #15675

Merged

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

This pull request was closed.

Comments

Conversation

xtqqczze commented Feb 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Context

PR Checklist

Uh oh!

vexx32 commented Feb 5, 2020

Uh oh!

Uh oh!

Uh oh!

iSazonov Feb 5, 2020

Choose a reason for hiding this comment

Uh oh!

xtqqczze Oct 12, 2020

Choose a reason for hiding this comment

Uh oh!

xtqqczze Oct 12, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iSazonov left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented May 27, 2020

Uh oh!

iSazonov commented May 27, 2020

Uh oh!

TravisEz13 commented Oct 25, 2020

Uh oh!

xtqqczze commented Oct 26, 2020

Uh oh!

iSazonov commented Nov 5, 2020

Uh oh!

xtqqczze commented Nov 5, 2020

Uh oh!

iSazonov commented Nov 5, 2020

Uh oh!

iSazonov commented Nov 21, 2020

Uh oh!

xtqqczze commented Nov 23, 2020

Uh oh!

xtqqczze commented Dec 6, 2020

Uh oh!

ghost commented Dec 24, 2020

Uh oh!

xtqqczze commented Jun 25, 2021

Uh oh!

xtqqczze commented Jun 25, 2021

Uh oh!

xtqqczze commented Jun 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

xtqqczze commented Feb 5, 2020 •

edited

Loading