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.
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
Unsealed IDisposable should contain protected virtual void Dispose(bool)
method
#11820
base: master
Are you sure you want to change the base?
Conversation
|
3f6c823
to
c56daa1
Compare
Adding committee review tag. I am not sure if these changes should be made. |
Perhaps it makes sense for public (not sealed) classes. |
@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. |
@xtqqczze Do you want to continue with reviewing public classes? |
@xtqqczze Friendly ping. |
63851c4
to
ea36c64
Compare
ea36c64
to
666b6c0
Compare
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
@xtqqczze Do you want to continue? |
@iSazonov yes please, thanks for the reminder |
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
protected virtual void Dispose(bool)
method
666b6c0
to
34b7535
Compare
Rebased for CI, this PR is ready for review. |
I would not change this old code without urgent need. /cc @daxian-dbw @PaulHigin for conclusion. |
There was a problem hiding this 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.
src/Microsoft.Management.Infrastructure.CimCmdlets/CimCommandBase.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/FormatAndOutput/common/BaseCommand.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/engine/remoting/client/ClientRemotePowerShell.cs
Show resolved
Hide resolved
src/System.Management.Automation/engine/remoting/client/remotingprotocolimplementation.cs
Show resolved
Hide resolved
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
The motivation of this PR is this comment by @PaulHigin PowerShell#11820 (comment). _Contributes to PowerShell#15110._
PR Summary
PR Context
Fix code smell: https://rules.sonarsource.com/csharp/RSPEC-3881
PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.