Update S927: C#: parameter names should match base declaration and other partial definitions #534
Conversation
valhristov
force-pushed
the
s927-parameter-names
branch
from
4e78abd
to
0bd46d4
Compare
| "issues": [ | ||
| { | ||
| "id": "S927", | ||
| "message": "Rename parameter 'pipelineItem' to 'syncDelegate'.", |
There was a problem hiding this comment.
While we're at the rule - perhaps tweak the message to give some context:
"Rename parameter '{0}' to '{1}' to match the inhertited parameter name."
(or something similar)
WDYT?
| } | ||
| else | ||
| { | ||
| // do nothing |
There was a problem hiding this comment.
Do we need that empty else? I know this is there intentionally to make point, but do we need that?
Perhaps rewrite to somethig like that - WDYT?
var parameters = methodSymbol?.PartialDefinitionPart?.Parameters ?? methodSymbol?.OverriddenMethod?.Parameters;
VerifyParameters(c, methodSyntax, expectedParameters: parameters);
There was a problem hiding this comment.
Good suggestion, but with the message argument I cannot do it nicely... See the updated code.
| private static void VerifyParameters(SyntaxNodeAnalysisContext context, | ||
| MethodDeclarationSyntax methodSyntax, IList<IParameterSymbol> expectedParameters) | ||
| { | ||
| var implementationParameters = methodSyntax.ParameterList.Parameters; |
There was a problem hiding this comment.
Do we know the collections are always the same length?
[Optional] you could try using Zip the way we did in MethodOverloadOptionalParameter.cs
There was a problem hiding this comment.
Yes, otherwise the methods will not be matched as overrides... I replaced the loop with Zip.
…her partial definitions
valhristov
force-pushed
the
s927-parameter-names
branch
from
0bd46d4
to
3b7543a
Compare
Fix #516