Skip to content

Conversation

@xtqqczze
Copy link
Contributor

@xtqqczze xtqqczze commented Oct 25, 2020

@ghost ghost assigned TravisEz13 Oct 25, 2020
@xtqqczze xtqqczze marked this pull request as ready for review October 26, 2020 16:37
@iSazonov
Copy link
Collaborator

Should an override be provided for GetHashCode() too?

It is not clear if we force the rule why does it not complain?

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

Should an override be provided for GetHashCode() too?

It is not clear if we force the rule why does it not complain?

We might expect to see compiler warning (level 3) CS0659 if the override for GetHashCode is missing. It appears we have level 5 warnings enabled.

@xtqqczze
Copy link
Contributor Author

Should an override be provided for GetHashCode() too?

It is not clear if we force the rule why does it not complain?

The method already exists: 😛
public override int GetHashCode()

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

ghost commented Nov 6, 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

@iSazonov iSazonov removed the CL-BreakingChange Indicates that a PR should be marked as a breaking change in the Change Log label Nov 21, 2020
@ghost ghost removed the Review - Needed The PR is being reviewed label Nov 21, 2020
@iSazonov
Copy link
Collaborator

I am not sure it is a breaking change.

@xtqqczze
Copy link
Contributor Author

xtqqczze commented Nov 22, 2020

I am not sure it is a breaking change.

According to .NET compatibility guidelines, probably not:

ALLOWED: Adding or removing an override

https://docs.microsoft.com/dotnet/core/compatibility/#members

Learn how .NET Core attempts to maintain compatibility for developers across .NET versions, and what kind of change is considered a breaking change.

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

ghost commented Nov 29, 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

@iSazonov iSazonov added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Jun 28, 2021
@iSazonov
Copy link
Collaborator

Ask PowerShell Committee about adding new public API.

@ghost ghost removed the Review - Needed The PR is being reviewed label Jun 28, 2021
@SteveL-MSFT
Copy link
Member

@PowerShell/powershell-committee reviewed this, we are ok taking the new public API, but a test needs to be added and we believe the current implementation may not be correct

@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 Jun 30, 2021
Co-authored-by: Joel Sallow (/u/ta11ow) <32407840+vexx32@users.noreply.github.com>
@iSazonov
Copy link
Collaborator

iSazonov commented Jul 1, 2021

@xtqqczze Please add some xUnit tests as Committee ask.

@xtqqczze
Copy link
Contributor Author

xtqqczze commented Jul 1, 2021

@iSazonov I do not know even where to begin with adding these tests :(

@iSazonov
Copy link
Collaborator

iSazonov commented Jul 1, 2021

@xtqqczze xUnit tests are in test\xUnit\csharp folder. (You could look similar tests for Equals() in .Net Runtime repo.)

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

ghost commented Jul 9, 2021

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

@TravisEz13 TravisEz13 removed the Review - Needed The PR is being reviewed label Aug 5, 2021
@TravisEz13 TravisEz13 changed the title Enable CA1067: Override Object.Equals(object) when implementing IEquatable<T> Enable CA1067: Override Object.Equals(object) when implementing IEquatable<T> Aug 5, 2021
@TravisEz13 TravisEz13 merged commit 01a4714 into PowerShell:master Aug 5, 2021
@TravisEz13 TravisEz13 added this to the 7.2.0-preview.9 milestone Aug 5, 2021
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 Committee-Reviewed PS-Committee has reviewed this and made a decision

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants