PowerShell / PowerShell Public
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
Seal ClientRemotePowerShell
#15802
Seal ClientRemotePowerShell
#15802
Conversation
@xtqqczze Please add more information in the PR description why we need this. |
done |
It seems wrong to have a Dispose function that doesn't do anything. Stream objects are closed through protocol messages and Dispose appears to be unneeded. However, this code is so old I hesitate to make any significant changes to it, for fear of regressions.
@PaulHigin Since we seal We only instantiate the class in sealed class |
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
/rebase |
Seal `System.Management.Automation.Runspaces.Internal.ClientRemotePowerShell`
Fix [CS0628: new protected member declared in sealed class](https://docs.microsoft.com/dotnet/csharp/misc/cs0628)
Fix [IDE0044: Add readonly modifier](https://docs.microsoft.com/dotnet/fundamentals/code-analysis/style-rules/ide0044)
633d09f
to
b09d6aa
Compare
Pull request was closed
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? |
@iSazonov Thanks for merging :) |
Handy links: |
Seal internal
System.Management.Automation.Runspaces.Internal.ClientRemotePowerShell
class as it not meant to be derived from.The motivation of this PR is this comment by @PaulHigin in #11820 (comment). Sealing the class enables fixing the disposable implementation as previously attempted in #11820.
Contributes to #15110.