-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Add noinlineerr linter #5826
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
Add noinlineerr linter #5826
Conversation
Hey, thank you for opening your first Pull Request ! |
In order for a pull request adding a linter to be reviewed, the linter and the PR must follow some requirements.
Pull Request Description
Linter
The Linter Tests Inside Golangci-lint
|
@ldez thank you for checklist. Fixed:
Quick question:
I thought they already do – did I miss something? Could you clarify what exactly should be renamed or matched? |
I detected at least one false-positive: func _() {
if ok, _ := strconv.ParseBool("1"); ok {
fmt.Println("ok")
}
} The linter is very limited: it only handles explicit |
Even if I don't like inlined I don't know what should exactly be implemented, but I think the topic needs to be approached from a broader perspective, maybe with options and more. |
@ldez Thanks for the feedback! Do you have any specific ideas in mind? Some thoughts I had: Adding an option to choose the preferred style (though I’m not sure if it makes sense to only prefer the inline variant). Adding autofix support — though that’s a bit more complex for me at the moment and would need some digging in. Happy to hear any other suggestions you might have! |
@ldez Also, just to clarify — did you mean adding an option like “Check for blank identifiers” (e.g. |
Hey! 👋 Thanks again for the feedback — I’ve addressed most of the points in the latest update. Here’s what’s changed: Changes since last review:Proper type check for error: Auto-fix support: Avoids false positives: Question: Options?Right now the linter enforces a strict rule (any inline Possible ideas:
Let me know if you think this is worth exploring, or if the linter should stay strict and opinionated. |
The proposed options are not what I expected.
I'm talking about a broader perspective of inlined |
About the broader perspective — I get your point, but I’d prefer to keep the scope limited to I’ve often seen inline error handling being overused or misused, whereas inline Init with other values is way less common — That said, building a separate linter to cover all inline |
6c44991
to
ae87644
Compare
ok, so we agree that if a new linter is added that handles the whole topic of inlined If we agree on that, we can accept this linter. |
Totally agree |
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
Adding noinlineerr linter.
Purpose - allow only one style of error handling.
Instead of:
Prefer more explicit and readable: