Skip to content

Conversation

@xtqqczze
Copy link
Contributor

@xtqqczze xtqqczze commented Oct 25, 2020

PR Summary

Breaking change: this change removes the implicit public constructor for PowerShellAssemblyLoadContextInitializer.

Fix CA1822: Mark members as static.

Justification:

  • There appears to be no use case for creating an instance of PowerShellAssemblyLoadContextInitializer.
  • The class only contains one static method: SetPowerShellAssemblyLoadContext

PR Context

PR Checklist

@iSazonov
Copy link
Collaborator

Breaking change: this change removes the public constructor for PowerShellAssemblyLoadContextInitializer.

@xtqqczze I don't see the change.

@xtqqczze
Copy link
Contributor Author

xtqqczze commented Oct 28, 2020

Breaking change: this change removes the public constructor for PowerShellAssemblyLoadContextInitializer.

@xtqqczze I don't see the change.

The removed constructor is the implicit parameterless constructor.

See https://docs.microsoft.com/dotnet/csharp/programming-guide/classes-and-structs/constructors#parameterless-constructors

@iSazonov
Copy link
Collaborator

iSazonov commented Oct 28, 2020

What is a benefit of the change?

@xtqqczze
Copy link
Contributor Author

It is a fix for CA1822: Mark members as static.

Documentation categorizes this as a performance rule, but I would add it is also a possible correctness issue: we should not allow creation of an instance of PowerShellAssemblyLoadContextInitializer when there is no use case.

@iSazonov iSazonov added the CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log label Oct 28, 2020
@xtqqczze
Copy link
Contributor Author

xtqqczze commented Nov 4, 2020

on the other hand, this is breaking/changing how this function should be called by PS hosts, which will cause unnecessary additional work to change hosting code.

Correct me if I'm wrong, but the only this could be breaking is if someone attempts to create an instance of PowerShellAssemblyLoadContextInitializer. This seems unlikely as usage examples in documentation treat it as if it were already a static class:

"For such hosting scenarios, the native host needs to bootstrap by calling [PowerShellAssemblyLoadContextInitializer.SetPowerShellAssemblyLoadContext]"

https://github.com/PowerShell/PowerShell/blob/master/docs/host-powershell/README.md#special-hosting-scenario-for-native-host

@anmenaga
Copy link

anmenaga commented Nov 4, 2020

Oh, sorry. For some reason I thought that it was an instance method. :) Please ignore my previous post.

@anmenaga anmenaga merged commit 3444595 into PowerShell:master Nov 4, 2020
@xtqqczze xtqqczze deleted the static-PowerShellAssemblyLoadContextInitializer branch November 4, 2020 02:38
@iSazonov iSazonov added this to the 7.2.0-preview.1 milestone Nov 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-CodeCleanup Indicates that a PR should be marked as a Code Cleanup change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants