Skip to content
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 S2328: "GetHashCode should not reference mutable fields" should report once per method #414

Merged
merged 3 commits into from
Jun 9, 2017

Conversation

Evangelink
Copy link
Contributor

Fix #346.

Copy link
Contributor

@valhristov valhristov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM


var secondaryLocations = GetAllFirstMutableFieldsUsed(c, fieldsOfClass, identifiers)
.Select(identifierSyntax => new SecondaryLocation(identifierSyntax.GetLocation(),
string.Format(SecondaryMessageFormat, identifierSyntax.Identifier.Text)))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This Select is a bit ugly and could be extracted into a method...

{
return;
}

var semanticModel = await context.Document.GetSemanticModelAsync(context.CancellationToken).ConfigureAwait(false);
var allFieldDeclarationTasks = identifiersToFix.Select(identifier => GetFieldDeclarationSyntax(semanticModel, identifier, context.CancellationToken));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should reduce our "line too long" rule limit to 130 chars. GitHub is showing scrollbar for longer lines...

Copy link
Contributor

@michalb-sonar michalb-sonar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Minor comments, consider them optional.

return line;
}

if (match.Groups["issueType"].Value.Equals("Noncompliant"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to:
match.Groups["issueType"].Value == "Noncompliant"

@@ -50,7 +50,8 @@ public static bool IsObjectEquals(this IMethodSymbol methodSymbol)

public static bool IsObjectGetHashCode(this IMethodSymbol methodSymbol)
{
return methodSymbol.IsOverride &&
return methodSymbol != null &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. We should do it for all other methods in this class.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michalb-sonar Yes I think so too but this refactoring doesn't belong to this PR that's why I haven't done it yet.

@Evangelink Evangelink merged commit f0931cc into master Jun 9, 2017
@Evangelink Evangelink deleted the S2328-report-once branch June 9, 2017 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants