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

refactor (rule/receiver-naming): replace AST walker by iteration over declarations #1169

Merged
merged 1 commit into from
Dec 7, 2024

Conversation

chavacava
Copy link
Collaborator

Simplifies receiver-naming rule by replacing AST walker by an iteration over global declarations

Comment on lines +64 to +82
if name == "_" {
failures = append(failures, lint.Failure{
Node: decl,
Confidence: 1,
Category: "naming",
Failure: "receiver name should not be an underscore, omit the name if it is unused",
})
continue
}

if name == "this" || name == "self" {
failures = append(failures, lint.Failure{
Node: decl,
Confidence: 1,
Category: "naming",
Failure: `receiver name should be a reflection of its identity; don't use generic names such as "this" or "self"`,
})
continue
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use a switch

You are checking the same variable three times, also it would allow to remove the two continue, as they will be useless

Suggested change
if name == "_" {
failures = append(failures, lint.Failure{
Node: decl,
Confidence: 1,
Category: "naming",
Failure: "receiver name should not be an underscore, omit the name if it is unused",
})
continue
}
if name == "this" || name == "self" {
failures = append(failures, lint.Failure{
Node: decl,
Confidence: 1,
Category: "naming",
Failure: `receiver name should be a reflection of its identity; don't use generic names such as "this" or "self"`,
})
continue
}
switch name {
case "_":
failures = append(failures, lint.Failure{
Node: decl,
Confidence: 1,
Category: "naming",
Failure: "receiver name should not be an underscore, omit the name if it is unused",
})
case "this", "self":
failures = append(failures, lint.Failure{
Node: decl,
Confidence: 1,
Category: "naming",
Failure: `receiver name should be a reflection of its identity; don't use generic names such as "this" or "self"`,
})
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The switch on name will cope with the first two conditions for failure, the other conditions are not just on name thus IMO, adding a switch will not help in clarity (further, even with a switch continue is needed after adding a failure to do not evaluate the not-in-the-switch conditions)

@chavacava chavacava merged commit 5c2aadf into master Dec 7, 2024
5 checks passed
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.

2 participants