-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Enable CA1067: Override Object.Equals(object) when implementing IEquatable<T>
#13871
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
Conversation
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 |
The method already exists: 😛 |
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
|
I am not sure it is a breaking change. |
According to .NET compatibility guidelines, probably not:
https://docs.microsoft.com/dotnet/core/compatibility/#members
|
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
|
Ask PowerShell Committee about adding new public API. |
|
@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 |
Co-authored-by: Joel Sallow (/u/ta11ow) <32407840+vexx32@users.noreply.github.com>
|
@xtqqczze Please add some xUnit tests as Committee ask. |
|
@iSazonov I do not know even where to begin with adding these tests :( |
|
@xtqqczze xUnit tests are in test\xUnit\csharp folder. (You could look similar tests for Equals() in .Net Runtime repo.) |
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
CA1067: Override Object.Equals(object) when implementing IEquatable<T>
https://docs.microsoft.com/dotnet/fundamentals/code-analysis/quality-rules/ca1067