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/UnusedComparison and Lint/UnusedLiteral #507

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

nobodywasishere
Copy link
Contributor

@nobodywasishere nobodywasishere commented Nov 22, 2024

Closes #235
Closes #508

This is still a little WIP, I need to go through the code again and write more / better specs. Maybe the visitor can be moved to the main visitors, as it may be useful for writing other Lint/Useless* rules. It's basically useful for checking if the return value from a node will be captured or not, which is how I determined whether the comparison operations were useful.

image

@Sija Sija added the rule label Nov 22, 2024
@Sija
Copy link
Member

Sija commented Nov 22, 2024

Nice one ❤️ I'd rename the rule to UnusedComparison though, useless IMO brings some negative connotations.

@Sija
Copy link
Member

Sija commented Nov 22, 2024

I'll do the code review once you mark the PR as ready for review, just FYI.

@nobodywasishere nobodywasishere changed the title Add Lint/UselessComparison Add Lint/UnusedComparison and Lint/UnusedLiteral Nov 23, 2024
@nobodywasishere
Copy link
Contributor Author

I'll do the code review once you mark the PR as ready for review, just FYI.

Yeah that's why I marked it as draft, so you wouldn't waste time reviewing it only for me to completely change it.

@nobodywasishere nobodywasishere marked this pull request as ready for review November 23, 2024 08:47
@nobodywasishere
Copy link
Contributor Author

Running these rules over the ameba, Kagi, and crystal compiler codebases, didn't turn up any violations. Don't know if that's due to a bug in the rules or violations being rare, though I'm leaning towards the latter.

@nobodywasishere
Copy link
Contributor Author

The only literal that isn't supported is RegexLiteral, and that's due to them not having a location.

@straight-shoota
Copy link
Contributor

Adding location to RegexLiteral in crystal-lang/crystal#15235

src/ameba/ast/visitors/implicit_return_visitor.cr Outdated Show resolved Hide resolved
src/ameba/ast/visitors/implicit_return_visitor.cr Outdated Show resolved Hide resolved
src/ameba/ast/visitors/implicit_return_visitor.cr Outdated Show resolved Hide resolved
src/ameba/ast/visitors/implicit_return_visitor.cr Outdated Show resolved Hide resolved
src/ameba/ast/visitors/implicit_return_visitor.cr Outdated Show resolved Hide resolved
src/ameba/ast/visitors/implicit_return_visitor.cr Outdated Show resolved Hide resolved
src/ameba/rule/lint/unused_comparison.cr Outdated Show resolved Hide resolved
src/ameba/rule/lint/unused_comparison.cr Outdated Show resolved Hide resolved
src/ameba/rule/lint/unused_literal.cr Outdated Show resolved Hide resolved
@Sija Sija added this to the 1.7.0 milestone Nov 30, 2024
spec/ameba/rule/lint/unused_literal_spec.cr Outdated Show resolved Hide resolved
spec/ameba/rule/lint/unused_literal_spec.cr Outdated Show resolved Hide resolved
spec/ameba/rule/lint/unused_literal_spec.cr Outdated Show resolved Hide resolved
spec/ameba/rule/lint/unused_literal_spec.cr Outdated Show resolved Hide resolved
spec/ameba/rule/lint/unused_literal_spec.cr Outdated Show resolved Hide resolved
spec/ameba/rule/lint/unused_literal_spec.cr Outdated Show resolved Hide resolved
src/ameba/ast/visitors/implicit_return_visitor.cr Outdated Show resolved Hide resolved
Co-authored-by: Sijawusz Pur Rahnama <[email protected]>
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.

Add Lint/UnusedLiteral Condition in void context
3 participants