-
Notifications
You must be signed in to change notification settings - Fork 468
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
Update messages for symbol comparison diagnostic and codefix #5535
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -341,13 +341,13 @@ | |
<value>Symbols should be compared for equality, not identity. Use an overload accepting an 'IEqualityComparer' and pass 'SymbolEqualityComparer'.</value> | ||
</data> | ||
<data name="CompareSymbolsCorrectlyMessage" xml:space="preserve"> | ||
<value>Compare symbols correctly</value> | ||
<value>Use 'SymbolEqualityComparer' when comparing symbols</value> | ||
</data> | ||
<data name="CompareSymbolsCorrectlyTitle" xml:space="preserve"> | ||
<value>Compare symbols correctly</value> | ||
<value>Symbols should be compared for equality</value> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I understand correctly, this is misleading. A code violating this rule is already comparing for equality. e,g The thing is mostly about the "intent" when there are nullable differences. |
||
</data> | ||
<data name="CompareSymbolsCorrectlyCodeFix" xml:space="preserve"> | ||
<value>Compare symbols correctly</value> | ||
<value>Use a 'SymbolEqualityComparer' for symbol comparison</value> | ||
</data> | ||
<data name="ConfigureGeneratedCodeAnalysisMessage" xml:space="preserve"> | ||
<value>Configure generated code analysis</value> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existing description (not touched in this PR) doesn't make much sense to me for the same reasoning in #5535 (comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Equality kind of stretches into inclusion/exclusion of nullable here. Shorthand to say "Should compare for equality" makes some sense to me, but I definitely see where the ", not identity" can be misleading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's exactly it. this is what should be indicated by the diagnostic.
Probably:
Make your symbol comparison intent explicit
Specify whether you want to include nullability when comparing symbols
When comparing symbols, explicitly specify a 'SymbolEqualityComparer' to indicate whether you want to include nullability in comparison
(I know my wording isn't very good, but something along those lines?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about:
Symbol comparison should use 'SymbolEqualityComparer'
Use a 'SymbolEqualityComparer' to specify whether to include nullability when comparing symbols
When comparing symbols, it's good practice to specify whether symbols with different nullability should be considered equal. Use a 'SymbolEqualityComparer' to specify in this comparison whether to include nullability as part of the equality check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That way the title tells you what we expect, the message and description both elaborate on the why
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much better!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback :) I'll see if I can get that in this afternoon