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

simple: S1008 suggests to remove comments from if()s that are specifically written to make a comment stand out #1488

Open
alexander-hirth opened this issue Jan 8, 2024 · 4 comments

Comments

@alexander-hirth
Copy link

Sometimes it makes sense to have code organised this way:

if cond {
    // explain a rather non-obvious condition checked by this if()
    return true
}
// I expect more cases to appear where I need to return true,
// but for now the above case is the only one.
return false

Gosimple's check S1008 suggests to replace this with a plain return cond.

If a conditional if smth { return true } has comments inside, then do not suggest to fold it with the code that follows.

@alexander-hirth alexander-hirth added false-positive needs-triage Newly filed issue that needs triage labels Jan 8, 2024
@dominikh dominikh added noisy-positive and removed needs-triage Newly filed issue that needs triage labels Jan 8, 2024
@Dynom
Copy link

Dynom commented Apr 4, 2024

Wouldn't this be a prime candidate to use the ignore tag's?

e.g.: //lint:ignore S1008 explain a rather non-obvious condition checked by this if() ?

@alexander-hirth
Copy link
Author

Of course not. //lint:ignore is positively ugly.

@Dynom
Copy link

Dynom commented Apr 4, 2024

To have the linter assume that when there are comments in place it's "OK", you already add comments. Adding a prefix to be more explicit about it instead of assuming sounds much more robust to me. Plus it allows you to find it in the future, something you've already planned for.

@Dynom
Copy link

Dynom commented Apr 4, 2024

Either way, reads like a dupe of #704

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants