Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
xtqqczze wants to merge 7 commits into from
Closed

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

xtqqczze wants to merge 7 commits into from

Conversation

@xtqqczze
Copy link
Contributor

@xtqqczze xtqqczze commented Feb 10, 2020 •
edited
Loading

PR Summary

PR Context

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

PR Checklist

@ghost ghost assigned iSazonov Feb 10, 2020
@xtqqczze
Copy link
Contributor Author

xtqqczze commented Feb 10, 2020 •
edited
Loading

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 PaulHigin added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Feb 18, 2020
@PaulHigin
Copy link
Contributor

PaulHigin commented Feb 18, 2020

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

@iSazonov
Copy link
Collaborator

iSazonov commented Feb 18, 2020

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

@SteveL-MSFT
Copy link
Member

SteveL-MSFT commented Feb 19, 2020

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

iSazonov commented Feb 22, 2020

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

iSazonov commented Mar 13, 2020

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

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

@iSazonov
Copy link
Collaborator

iSazonov commented May 27, 2020

@xtqqczze Do you want to continue?

@TravisEz13 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
@iSazonov
Copy link
Collaborator

iSazonov commented Jul 9, 2020

I would not change this old code without urgent need.

/cc @daxian-dbw @PaulHigin for conclusion.

@ghost ghost removed the Review - Needed The PR is being reviewed label Jul 9, 2020
PaulHigin
PaulHigin reviewed Jul 9, 2020
View reviewed changes
Copy link
Contributor

@PaulHigin PaulHigin left a comment

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

@ghost ghost added the Review - Needed The PR is being reviewed label Jul 17, 2020
@ghost
Copy link

ghost commented Jul 17, 2020

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

iSazonov commented Mar 4, 2025

Close as very old.

@iSazonov iSazonov closed this Mar 4, 2025
@iSazonov 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
Copy link
Contributor

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

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

🔗 https://aka.ms/PSRepoFeedback

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

@SteveL-MSFT SteveL-MSFT Awaiting requested review from SteveL-MSFT

@TravisEz13 TravisEz13 Awaiting requested review from TravisEz13 TravisEz13 will be requested when the pull request is marked ready for review TravisEz13 is a code owner

1 more reviewer

@PaulHigin 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