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

Add Lint/NotNil rule #288

Merged
merged 2 commits into from
Oct 30, 2022
Merged

Add Lint/NotNil rule #288

merged 2 commits into from
Oct 30, 2022

Conversation

Sija
Copy link
Member

@Sija Sija commented Oct 30, 2022

Resolves #284

@Sija Sija added the rule label Oct 30, 2022
@Sija Sija requested a review from veelenga October 30, 2022 17:29
@Sija Sija self-assigned this Oct 30, 2022
src/ameba/rule/lint/not_nil.cr Outdated Show resolved Hide resolved
src/ameba/rule/lint/not_nil.cr Outdated Show resolved Hide resolved
src/ameba/rule/lint/percent_array.cr Outdated Show resolved Hide resolved
Copy link
Member

@veelenga veelenga left a comment

Choose a reason for hiding this comment

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

Lgtm

@Sija Sija added this to the 1.3.0 milestone Oct 30, 2022
@Sija Sija merged commit 038a365 into master Oct 30, 2022
@Sija Sija deleted the Sija/lint-not-nil-rule branch October 30, 2022 22:50
@Blacksmoke16
Copy link
Contributor

Blacksmoke16 commented Nov 13, 2022

@Sija @veelenga Do we think this is really something we should have enabled by default? I don't think a blanket "not_nil! is bad" without taking the context into consideration, which a linter is unable to do, is providing anything real meaningful.

Maybe for someone new to the language it might sway them to do something different, but then everyone else who understands when/how to use it is forced to either go thru their code and add ameba:disable Lint/NotNil everywhere, or disable the rule entirely themselves.

E.g. https://github.com/athena-framework/athena/actions/runs/3398343461/jobs/5651304890. I'd rather not go thru all of these cases to properly handle nil just to make ameba happy when I know something the compiler doesn't.

@Sija
Copy link
Member Author

Sija commented Nov 13, 2022

@Blacksmoke16 I was hesitant myself at the beginning, yet after checking this against several repos (incl. crystal), most of the issues were legitimate refactoring opportunities - and IMO these would be:

  • missing assignment to a local var: @foo.not_nil!
  • missing proper error handling: foo.not_nil! vs raise ... unless foo

Remaining ones, incl. cases in which using not_nil! is the only sensible thing to do, turned out to be a small minority, easily disabled with # ameba:disable pragma - and I regard this as a positive thing, since you're giving by-watchers a signal about your intentions.

All in all, using not_nil! is a code-smell, so all of the cases should be carefully decided upon, IMO even at the cost of potential brief annoyance.

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

Successfully merging this pull request may close these issues.

Warning on #not_nil! usage
3 participants