Skip to content

Unsealed IDisposable should contain protected virtual void Dispose(bool) method - #11820

Closed
xtqqczze wants to merge 7 commits into
PowerShell:masterfrom
xtqqczze:patch-dispose-virtual-method
Closed

Unsealed IDisposable should contain protected virtual void Dispose(bool) method#11820
xtqqczze wants to merge 7 commits into
PowerShell:masterfrom
xtqqczze:patch-dispose-virtual-method

Conversation

@xtqqczze

@xtqqczze xtqqczze commented Feb 10, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

PR Summary

PR Context

Fix code smell: https://rules.sonarsource.com/csharp/RSPEC-3881

PR Checklist

@ghost ghost assigned Ilya (iSazonov) Feb 10, 2020
@xtqqczze

xtqqczze commented Feb 10, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

PowerShell-CI-linux failed in build 45608: Invoke-WebRequest tests.Denial of service.Image Parsing Issue opened: #11821

@xtqqczze
xtqqczze force-pushed the patch-dispose-virtual-method branch from 3f6c823 to c56daa1 Compare February 10, 2020 19:39
@PaulHigin Paul Higinbotham (PaulHigin) added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Feb 18, 2020
@PaulHigin

Paul Higinbotham (PaulHigin) commented Feb 18, 2020

Copy link
Copy Markdown
Contributor

Adding committee review tag. I am not sure if these changes should be made.

@iSazonov

Ilya (iSazonov) commented Feb 18, 2020

Copy link
Copy Markdown
Collaborator

Perhaps it makes sense for public (not sealed) classes.

@SteveL-MSFT

Steve Lee (SteveL-MSFT) commented Feb 19, 2020

Copy link
Copy Markdown
Member

@PowerShell/powershell-committee reviewed this, we should not make any unnecessary changes to private/internal classes. We should review public classes if the change makes sense. Additionally, going forward, we would appreciate if the summary section is filled out to provide context on the reasons for making this change and enable the committee to have more productive conversations during review.

@SteveL-MSFT Steve Lee (SteveL-MSFT) added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Feb 19, 2020
@xtqqczze xtqqczze closed this Feb 21, 2020
@iSazonov

Ilya (iSazonov) commented Feb 22, 2020

Copy link
Copy Markdown
Collaborator

xtqqczze Do you want to continue with reviewing public classes?

@xtqqczze xtqqczze reopened this Feb 22, 2020
@xtqqczze xtqqczze changed the title Fix IDisposable implementation by defining Dispose method as virtual WIP: Fix IDisposable implementation by defining Dispose method as virtual Feb 22, 2020
@iSazonov

Ilya (iSazonov) commented Mar 13, 2020

Copy link
Copy Markdown
Collaborator

xtqqczze Friendly ping.

@xtqqczze
xtqqczze force-pushed the patch-dispose-virtual-method branch 3 times, most recently from 63851c4 to ea36c64 Compare April 10, 2020 22:46
@xtqqczze xtqqczze changed the title WIP: Fix IDisposable implementation by defining Dispose method as virtual WIP: Fix IDisposable implementation Apr 10, 2020
@xtqqczze
xtqqczze force-pushed the patch-dispose-virtual-method branch from ea36c64 to 666b6c0 Compare April 10, 2020 23:19
@ghost ghost added the Review - Needed The PR is being reviewed label May 27, 2020
@ghost

Deleted user (ghost) commented May 27, 2020

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

Ilya (iSazonov) commented May 27, 2020

Copy link
Copy Markdown
Collaborator

xtqqczze Do you want to continue?

@TravisEz13 Travis Plunk (TravisEz13) added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Review - Needed The PR is being reviewed Review - Waiting on Author labels May 27, 2020
@xtqqczze xtqqczze changed the title WIP: Fix IDisposable implementation Fix IDisposable implementation May 28, 2020
@ghost ghost removed the Review - Needed The PR is being reviewed label Jul 9, 2020
PaulHigin
Paul Higinbotham (PaulHigin) reviewed Jul 9, 2020
View reviewed changes

@PaulHigin Paul Higinbotham (PaulHigin) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I looked through these changes and agree with Ilya (@iSazonov) that they don't really provide much value with some small risk. Some classes are internal and should be 'sealed' because they are not meant to be derived from. Others are public and also should probably be sealed.

I appreciate the effort to find and make these changes, but I feel we shouldn't submit the changes because there is no compelling reason to.

Comment thread src/Microsoft.Management.Infrastructure.CimCmdlets/CimCommandBase.cs Resolved
Comment thread src/System.Management.Automation/FormatAndOutput/common/BaseCommand.cs Resolved
Comment thread src/System.Management.Automation/engine/remoting/client/ClientRemotePowerShell.cs Resolved
Comment thread src/System.Management.Automation/engine/remoting/client/remotingprotocolimplementation.cs Resolved
@ghost ghost added the Review - Needed The PR is being reviewed label Jul 17, 2020
@ghost

Deleted user (ghost) commented Jul 17, 2020

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.
Maintainer, please provide feedback and/or mark it as Waiting on Author

This was referenced Jul 18, 2021
Merged
Merged
xtqqczze added a commit to xtqqczze/PowerShell-PowerShell that referenced this pull request Feb 13, 2024
@xtqqczze
Seal ClientRemoteSessionDSHandlerImpl
5b61869 
The motivation of this PR is this comment by @PaulHigin PowerShell#11820 (comment).

_Contributes to PowerShell#15110._
@xtqqczze xtqqczze mentioned this pull request Feb 13, 2024
Merged
@xtqqczze xtqqczze mentioned this pull request Dec 24, 2024
Merged
Ilya (iSazonov) pushed a commit that referenced this pull request Dec 28, 2024
@xtqqczze
Seal ClientRemoteSessionDSHandlerImpl (#21218)
b74e708 
The motivation of this PR is this comment by @PaulHigin #11820 (comment).

_Contributes to #15110._
@xtqqczze
xtqqczze marked this pull request as draft January 18, 2025 13:52
@microsoft-github-policy-service microsoft-github-policy-service Bot added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Jan 22, 2025
@microsoft-github-policy-service microsoft-github-policy-service Bot 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 Jan 31, 2025
@xtqqczze xtqqczze closed this Feb 19, 2025
@xtqqczze xtqqczze reopened this Feb 19, 2025
@microsoft-github-policy-service microsoft-github-policy-service Bot 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 Feb 19, 2025
@iSazonov

Ilya (iSazonov) commented Mar 4, 2025

Copy link
Copy Markdown
Collaborator

Close as very old.

@iSazonov Ilya (iSazonov) removed their assignment Mar 4, 2025
@microsoft-github-policy-service microsoft-github-policy-service Bot removed the Review - Needed The PR is being reviewed label Mar 4, 2025
@microsoft-github-policy-service

microsoft-github-policy-service Bot commented Mar 4, 2025 •
edited by unfurl-links Bot
Loading

Copy link
Copy Markdown
Contributor

📣 Hey xtqqczze, how did we do? We would love to hear your feedback with the link below! 🗣️

🔗 https://aka.ms/PSRepoFeedback

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@xtqqczze
Seal ClientRemoteSessionDSHandlerImpl (PowerShell#21218)
50939ea 
The motivation of this PR is this comment by @PaulHigin PowerShell#11820 (comment).

_Contributes to PowerShell#15110._
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daxian-dbw Dongbo Wang (daxian-dbw) Awaiting requested review from daxian-dbw daxian-dbw is a code owner
@SteveL-MSFT Steve Lee (SteveL-MSFT) Awaiting requested review from SteveL-MSFT
@TravisEz13 Travis Plunk (TravisEz13) Awaiting requested review from TravisEz13 TravisEz13 will be requested when the pull request is marked ready for review TravisEz13 is a code owner
Copilot code review Copilot
Copilot will automatically review once the pull request is marked ready for review
1 more reviewer
@PaulHigin Paul Higinbotham (PaulHigin) PaulHigin left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Committee-Reviewed PS-Committee has reviewed this and made a decision Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@xtqqczze @PaulHigin @iSazonov @SteveL-MSFT @TravisEz13