Skip to content

Convert public field to property - #14196

Closed
xtqqczze wants to merge 2 commits into
PowerShell:masterfrom
xtqqczze:CA2211-IsServerManager
Closed

Convert public field to property#14196
xtqqczze wants to merge 2 commits into
PowerShell:masterfrom
xtqqczze:CA2211-IsServerManager

Conversation

@xtqqczze

@xtqqczze xtqqczze commented Nov 20, 2020

Copy link
Copy Markdown
Contributor

Convert public field PSSessionConfigurationData.IsServerManager to property.

Fix CA2211: Non-constant fields should not be visible.

Breaking change.

@xtqqczze
xtqqczze marked this pull request as ready for review November 20, 2020 09:49
@iSazonov

Copy link
Copy Markdown
Collaborator

Breaking change in public API?

@xtqqczze

xtqqczze commented Nov 23, 2020

Copy link
Copy Markdown
Contributor Author

Breaking change in public API?

Yes and probably unacceptable, but I would like the change to be considered.


/// <summary>
/// Gets or sets a value indicating whether Server Manager is enabled.
/// </summary>

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 am concerned about backward compatibility when converting a public field into a public property.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yes, I think we break a binary compatibility.

@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Nov 23, 2020
@iSazonov

Copy link
Copy Markdown
Collaborator

xtqqczze I think we can not approve the change. If you want force the rule please use local suppression for IsServerManager.

@xtqqczze

Copy link
Copy Markdown
Contributor Author

unacceptable breaking change, closing.

@xtqqczze xtqqczze closed this Nov 25, 2020
@ghost ghost removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Nov 25, 2020
@xtqqczze
xtqqczze deleted the CA2211-IsServerManager branch November 25, 2020 08:38
@iSazonov Ilya (iSazonov) added the Breaking-Change breaking change that may affect users label Nov 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Breaking-Change breaking change that may affect users

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants