-
Notifications
You must be signed in to change notification settings - Fork 509
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
Implement SA1314 (TypeParameterNamesMustBeginWithT) #1925
Conversation
Current coverage is 96.88% (diff: 98.01%)@@ master #1925 diff @@
==========================================
Files 586 589 +3
Lines 80814 81066 +252
Methods 3487 3497 +10
Messages 0 0
Branches 3090 3098 +8
==========================================
+ Hits 78295 78542 +247
- Misses 1680 1682 +2
- Partials 839 842 +3
|
❗ naming rules have IDs with the form SA13xx. ❓ should we refer to "type parameters" instead of "generic parameters"? |
Type parameters would be preferred for me |
I wasn't sure myself; I'd be totally fine with changing it to "type parameters" across the board. |
Fixed both the ID and the rule name. 👍 |
@@ -52,6 +52,20 @@ internal static class RenameHelper | |||
|
|||
var containingSymbol = symbol.ContainingSymbol; | |||
|
|||
if (symbol.Kind == SymbolKind.TypeParameter) |
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.
We cant rename the type parameter if there is already a type with the same name
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.
You mean (for example) if there was already a class named TKey
?
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.
Exactly
Maybe we should include a test to verify that this also works for type parameters in method signatures |
@pdelvo Added a test case for type parameters in method signatures, and also refactored the logic in |
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.
There were two conflicts with the latest master branch, but I went ahead and fixed them:
- Minor conflict where the version number in the solution file was updated in two different branches (resolved to the most recent one)
- API conflict related to registering analysis callbacks (resolved by updating SA1314 implementation to use the new pattern)
I also fixed two trivial style issues for consistency.
Fixes #739. Includes code fix, tests, and documentation.
Let me know if there's any test scenarios I didn't think of!