Consider disabling `Style/InvertibleUnlessCondition`?

[andyw8]

(following up from internal Slack discussion)

This rule is disabled by default in RuboCop's config. It was enabled in https://github.com/Shopify/ruby-style-guide/pull/486.

It's causing some problems since it can contradict Style/NegatedIf.

Also it does not match the styleguide rule for "Favour unless over if for negative conditions".

[sambostock]

It's causing some problems since it can contradict Style/NegatedIf.

Also it does not match the styleguide rule for "Favour unless over if for negative conditions".

This is a misunderstanding of what the cop is suggesting:

# bad
foo unless n.odd?

# also bad (supposed contradiction)
foo if !n.odd?

# good
foo if n.even?

I've opened https://github.com/rubocop/rubocop/pull/12562 to improve the offense message to make this clearer.

-Favor `if` with inverted condition over `unless`.
+Prefer `if n.even?` over `unless n.odd?`.

[Chris911]

What about when there's no inverse method like for include?

This issue was opened because there's no good alternative to: foo unless set.include?(bar). InvertibleUnlessCondition marks this as a violation and suggests using foo if !set.include?(bar) which then becomes a violation of NegatedIf.

Looking at the rule implementation, I don't understand why it considers using include? with unless as a violation as there's no inverse method (InverseMethods) for include.

[sambostock]

Transcribing my comments on the Slack thread, where I addressed those points:

as fair as I know there’s no exclude? (or an equivalent) on either Array or Set

Rails adds that on Enumerable, which Array and Set both are, and rubocop-rails teaches rubocop about it. rubocop won't complain about it if rubocop-rails isn't installed.

---

So the problem there is that rubocop has no way to know that that file is run in a context where Rails isn't loaded, and Enumerable#exclude? isn't available.

Style/InvertibleUnlessCondition: Favor if with inverted condition over unless.

is never suggesting adding a !, it's always suggesting using the inverted method (e.g. odd? vs even?, not !odd?).

---

If you're in a context where that method doesn't exist, then:

• it would be appropriate to disable the cop, ideally with a comment explaining why.
• it would be preferable to find a way to teach RuboCop not to make the mistake, to minimize disabling cops.
• (can provide custom config for that directory?)

---

I'll open a PR proposing a change to that offense message though, since this isn't the first time I've seen it confuse people. I think something like

Style/InvertibleUnlessCondition: Favor if with the inverse method over unless.

or maybe even including the recommended method would be preferable.

---

Favour unless over if for negative conditions. means favour

unless condition?

over

 if !condition?

---

I am, of course, open to a discussion about disabling the cop, but I think there are arguments for keeping it enabled:

• odd? & even? are good examples of when it enhances readability and use of domain terminology
• rails/rails#49909 added present? implementations which skip !blank?, which is a good example of cases where there is an optimization argument (here skipping an extra method call)