-
Notifications
You must be signed in to change notification settings - Fork 231
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
S927 FP fix: Fix cases when generic type arguments are changed #5241
The head ref may contain hidden characters: "\u010Daba/S927-FP-fix"
Conversation
return true; | ||
} | ||
} | ||
return false; |
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.
This line should not be needed.
{ | ||
context.ReportIssue(Diagnostic.Create(rule, parameter.GetLocation(), parameter.ValueText, expectedParameter.Name, expectedLocation)); | ||
} | ||
} | ||
} | ||
|
||
private static bool AreGenericWithDifferentTypes(ITypeSymbol actualType, ITypeSymbol expectedType) |
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.
I would change this name to something like AreGenericTypeParametersWithDifferentTypes
. To be a bit more clear what the method does.
WDYT?
if (actualType is INamedTypeSymbol actualNamedType | ||
&& expectedType is INamedTypeSymbol expectedNamedType) | ||
{ | ||
for (var i = 0; i < actualNamedType.TypeArguments.Count(); i++) |
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.
For readability maybe extract lines 109-115 in a different method?
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.
Seems ok, a couple of comments.
Kudos, SonarCloud Quality Gate passed! |
Kudos, SonarCloud Quality Gate passed! |
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.
LGTM
Fixes #4370