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

Replace Performance/AnyInsteadOfEmpty with Style/EmptyInsteadOfPresent #408

Open
devnote-dev opened this issue Oct 6, 2023 · 9 comments

Comments

@devnote-dev
Copy link

devnote-dev commented Oct 6, 2023

With crystal-lang/crystal#13866 going through, I believe that Performance/AnyInsteadOfEmpty should be replaced with Style/EmptyInsteadOfPresent which would check for double negation in if/unless statements and unnecessary ! operations in ternaries. For example:

if !{...}.empty? # should be rewritten to `unless {...}.empty?`
# or `if {...}.present?` if there is an `else`
unless !{...}.empty? # ditto

I'm placing this specifically as a Style rule because it has no bearing on performance, as proven by the forum thread for AnyInsteadOfEmpty. Alternatively it could be a Lint rule but I'm not sure what that group is for.

@devnote-dev
Copy link
Author

It would help if I got the names right... 😅

@devnote-dev devnote-dev changed the title Replace Performance/AnyInsteadOfEmpty with Style/AnyInsteadOfPresent Replace Performance/AnyInsteadOfEmpty with Style/EmptyInsteadOfPresent Oct 6, 2023
@Sija Sija added the rule label Oct 6, 2023
@Sija
Copy link
Member

Sija commented Oct 6, 2023

I believe two things should happen, once the above mentioned PR is merged:

  1. Rule Performance/AnyInsteadOfEmpty should become Style/AnyInsteadOfPresent
  2. Rule Style/EmptyInsteadOfPresent should be added
    Side note: there are cases in which the negated form is more natural - like in unless, so this new rule should take that into account

@Sija Sija added the refactor label Oct 6, 2023
@devnote-dev
Copy link
Author

If Style/AnyInsteadOfPresent is disabled by default then I'm fine with that. I don't know if Enumerable#any? is used commonly but I'd imagine it's a pain for people that also use Ameba in their project, they either have to refactor their code or disable the rule because at present, no semantic analysis is being done to determine that .any? is what Ameba interprets it to be.

@Sija
Copy link
Member

Sija commented Oct 6, 2023

@devnote-dev If it wasn't a pain already, then why would it be now? I don't know the numbers, but just looking on the amount of issues re: that - not too many projects are affected - which I find reasonable, since it's not that common to have #any? method defined.

@devnote-dev
Copy link
Author

True, I say this because I ran into this issue a couple months ago and just renamed the method to get around it, but I don't think it's something Ameba should do principally.

@Sija
Copy link
Member

Sija commented Oct 6, 2023

In such a case I'd advise to simply exclude this rule on the project level (.ameba.yml) instead - perfectly proper thing to do (see Lint/NotNil as the common, purposeful, offender) :)

@straight-shoota
Copy link
Contributor

Rule Performance/AnyInsteadOfEmpty should become Style/AnyInsteadOfPresent

There's also an option for Enumerable#any? (without param or block) to become deprecated. At that point this rule wouldn't be necessary I suppose? Or maybe it could provide automatic migration?

@Sija
Copy link
Member

Sija commented Oct 7, 2023

Automatic migration is certainly possible.

@HertzDevil
Copy link

The blockless #any? being callable most certainly implies #empty? is also callable, but note that crystal-lang/crystal#13866 is restricted to Enumerable and there are quite a few stdlib types that define #empty? but aren't Enumerable, like String and IO::Memory. Also in String's case !str.empty? is not equivalent to str.present? if it gains a #present? that has the same semantics as ActiveSupport.

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

4 participants