Enable Style/InvertibleUnlessCondition cop #15265

dduugg · 2023-04-18T20:14:38Z

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same change?
Have you added an explanation of what your changes do and why you'd like us to include them?
Have you written new tests for your changes? Here's an example.
Have you successfully run brew style with your changes locally?
Have you successfully run brew typecheck with your changes locally?
Have you successfully run brew tests with your changes locally?

This is a new cop introduced in rubocop v1.44.0. It's disabled by default but i think it's enough of a readability improvement to enable. (I also added a rule to favor if a != b over unless a == b.)

It is invalid in at least one case – when comparing class hierarchies that do not share inheritance (see the disable below).

cc @issyl0

dduugg · 2023-04-18T20:16:20Z

Library/Homebrew/cmd/list.rb

-      else
-        remaining_root_files << pn unless pn.basename.to_s == ".DS_Store"
+      elsif pn.basename.to_s != ".DS_Store"
+        remaining_root_files << pn


This is an especially nice fix, imo

Yes, else ... unless was weird!

dduugg · 2023-04-18T21:13:47Z

Currently failing CI due to upstream violations, will hold off on fixing until initial feedback is in

issyl0

Thanks @dduugg, nice discovery. I have some (opinionated) comments about the corrections here.

issyl0 · 2023-04-18T21:14:07Z

Library/.rubocop.yml

+  Enabled: true
+  InverseMethods:
+    # Favor `if a != b` over `unless a == b`
+    :==: :!=


I'm very surprised this isn't a default.

issyl0 · 2023-04-18T21:14:59Z

Library/Homebrew/cask/audit.rb

@@ -526,7 +526,7 @@ def audit_livecheck_version
    def audit_livecheck_min_os
      return unless online?
      return unless cask.livecheckable?
-      return unless cask.livecheck.strategy == :sparkle
+      return if cask.livecheck.strategy != :sparkle


Mmm, not sure how I feel about this when there's mixed unless and if in a row.

There's no easy general fix here that i can think of. Sometimes I chain these (return unless a && b && c), but i generally only use unless when rubocop makes me.

Let's keep it the way the cop wants it then.

I also prefer the cop's approach here.

issyl0 · 2023-04-18T21:16:02Z

Library/Homebrew/cmd/cleanup.rb

@@ -57,7 +57,7 @@ def cleanup

    cleanup.clean!

-    unless cleanup.disk_cleanup_size.zero?
+    if cleanup.disk_cleanup_size.nonzero?


I think I prefer this how it was? (And below in all these .nonzero? cases.)

That's a reasonable objection, i'll unset the entry that does this and re-apply the cop

issyl0 · 2023-04-18T21:20:13Z

Library/Homebrew/utils/analytics.rb

@@ -473,7 +473,7 @@ def table_output(category, days, results, os_version: false, cask_install: false
               "#{formatted_count} | #{formatted_percent}%"
          next if index > 10
        end
-        return unless results.length > 1
+        return if results.length <= 1


I think I prefer unless in all the cases where otherwise we have to replace > or < with the equals variant, the "unless" is easier to read (!) than "which way is that angle bracket going paired with that equals".

Not so for me (mostly bc i still have to mentally convert unless to if not, which makes comparisons like this a 🤕), but we could also unset these in the config, like for the zero? case.

I'll let a third person weigh in and maybe we'll get a majority vote one way or the other.

I also prefer the way the cop handles this.

issyl0 · 2023-04-18T21:21:31Z

Library/Homebrew/cmd/list.rb

-      else
-        remaining_root_files << pn unless pn.basename.to_s == ".DS_Store"
+      elsif pn.basename.to_s != ".DS_Store"
+        remaining_root_files << pn


Yes, else ... unless was weird!

MikeMcQuaid

Love this cop, enabling it on personal projects too, thanks @dduugg!

MikeMcQuaid · 2023-04-19T09:11:24Z

As a general note: I have on my "one day" TODO list to go through literally every disabled by default cop and try to enable them all and remove the ones I disagree with. Here's a list from a work project I've done this on, if it's useful:

# Enable some useful rules disabled by default.
Layout/ClassStructure:
  Enabled: true
Layout/EmptyLineAfterMultilineCondition:
  Enabled: true
Layout/FirstArrayElementLineBreak:
  Enabled: true
Layout/FirstHashElementLineBreak:
  Enabled: true
Layout/FirstMethodParameterLineBreak:
  Enabled: true
Layout/HeredocArgumentClosingParenthesis:
  Enabled: true
Layout/MultilineArrayLineBreaks:
  Enabled: true
Layout/MultilineHashKeyLineBreaks:
  Enabled: true
Layout/MultilineMethodParameterLineBreaks:
  Enabled: true
Layout/SingleLineBlockChain:
  Enabled: true
Lint/HeredocMethodCallPosition:
  Enabled: true
Naming/InclusiveLanguage:
  Enabled: true
Style/AutoResourceCleanup:
  Enabled: true
Style/CollectionMethods:
  Enabled: true
Style/ImplicitRuntimeError:
  Enabled: true
Style/InvertibleUnlessCondition:
  Enabled: true
Style/IpAddresses:
  Enabled: true
Style/OptionHash:
  Enabled: true
Style/ReturnNil:
  Enabled: true
Style/StringHashKeys:
  Enabled: true
Style/Send:
  Enabled: true
Style/StringMethods:
  Enabled: true
Style/TopLevelMethodDefinition:
  Enabled: true
Style/UnlessLogicalOperators:
  Enabled: true
Style/YodaExpression:
  Enabled: true
Performance/ChainArrayAllocation:
  Enabled: true
Performance/IoReadlines:
  Enabled: true
Performance/OpenStruct:
  Enabled: true
Performance/SelectMap:
  Enabled: true
Performance/CaseWhenSplat:
  Enabled: true
Rails/DefaultScope:
  Enabled: true
Rails/EnvironmentVariableAccess:
  Enabled: true
Rails/OrderById:
  Enabled: true
Rails/PluckId:
  Enabled: true
Rails/RequireDependency:
  Enabled: true
Rails/ReversibleMigrationMethodDefinition:
  Enabled: true
Rails/SaveBang:
  Enabled: true
Rails/TableNameAssignment:
  Enabled: true
RSpec/Dialect:
  Enabled: true
RSpec/MessageExpectation:
  Enabled: true
RSpec/Pending:
  Enabled: true
Sorbet/CallbackConditionalsBinding:
  Enabled: true
Sorbet/EnforceSignatures:
  Enabled: true
Sorbet/ForbidIncludeConstLiteral:
  Enabled: true
Sorbet/ForbidSuperclassConstLiteral:
  Enabled: true
Sorbet/ForbidTUnsafe:
  Enabled: true
Sorbet/OneAncestorPerLine:
  Enabled: true
Sorbet/SingleLineRbiClassModuleDefinitions:
  Enabled: true

issyl0 · 2023-04-19T09:15:40Z

@MikeMcQuaid Don't tempt me to do your TODO list items for you. It's a good idea.

reitermarkus · 2023-04-20T17:51:53Z

Library/Homebrew/cask/audit.rb

+      return if count <= 1

      add_error "only a single postflight stanza is allowed"


Suggested change

return if count <= 1

add_error "only a single postflight stanza is allowed"

add_error "only a single postflight stanza is allowed" if count > 1

Unless RuboCop wants a guard-clause here.

dduugg · 2023-04-20T18:13:04Z

@MikeMcQuaid Don't tempt me to do your TODO list items for you. It's a good idea.

Note that per comment on the rubocop PR, rubocop will be enabling fewer of these kinds of cops going forward. If you'd prefer to stay on top of these as they come in, it may be worth exploring the EnabledByDefault config setting.

MikeMcQuaid · 2023-04-21T07:26:01Z

Note that per comment on the rubocop PR, rubocop will be enabling fewer of these kinds of cops going forward. If you'd prefer to stay on top of these as they come in, it may be worth exploring the EnabledByDefault config setting.

Good idea. I knew about NewCops: enable (which we've enabled) but I do think EnabledByDefault probably makes more sense. When new cops are added and we have no violations: I think it's worth us enabling them.

issyl0 · 2023-04-21T23:27:56Z

As long as it's not DisabledByDefault, I'm game for config changes. Bad news though, currently running brew style after setting EnabledByDefault: true reveals 1170 files inspected, 42077 offenses detected, 21649 offenses autocorrectable. 💀 But that's why Mike is wise and said "only enable new cops if no offenses" (or, I guess, minimal, sensible, easily fixable ones). I'll make an issue over the weekend for determining which ones are good/bad/enforceable.

dduugg · 2023-04-22T02:42:23Z

As long as it's not DisabledByDefault, I'm game for config changes. Bad news though, currently running brew style after setting EnabledByDefault: true reveals 1170 files inspected, 42077 offenses detected, 21649 offenses autocorrectable. 💀 But that's why Mike is wise and said "only enable new cops if no offenses" (or, I guess, minimal, sensible, easily fixable ones). I'll make an issue over the weekend for determining which ones are good/bad/enforceable.

I'm not too surprised by that. It might be more worthwhile to attempt it by department (e.g. Style) and not globally.

dduugg commented Apr 18, 2023

View reviewed changes

issyl0 reviewed Apr 18, 2023

View reviewed changes

dduugg force-pushed the enable-InvertibleUnlessCondition branch 2 times, most recently from f5d70e9 to 6a7e67d Compare April 18, 2023 22:24

MikeMcQuaid approved these changes Apr 19, 2023

View reviewed changes

MikeMcQuaid enabled auto-merge April 19, 2023 09:11

This was referenced Apr 19, 2023

Apply Style/InvertibleUnlessCondition cop Homebrew/homebrew-core#128790

Merged

Apply Style/InvertibleUnlessCondition cop Homebrew/homebrew-bundle#1196

Merged

dduugg force-pushed the enable-InvertibleUnlessCondition branch from 6a7e67d to 4801d7c Compare April 20, 2023 16:40

reitermarkus reviewed Apr 20, 2023

View reviewed changes

issyl0 mentioned this pull request Apr 23, 2023

rubocop: Investigate setting EnabledByDefault and fixing sensible cops #15297

Closed

1 task

dduugg added 4 commits April 25, 2023 09:26

Enable Style/InvertibleUnlessCondition cop

cccbb5c

brew style --fix

08af78a

Disable false positive

89eb891

Simplify config

bf8ac8a

dduugg mentioned this pull request Apr 25, 2023

Resolve Style/InvertibleUnlessCondition violations Homebrew/homebrew-cask#145823

Merged

3 tasks

dduugg force-pushed the enable-InvertibleUnlessCondition branch from 4801d7c to bf8ac8a Compare April 26, 2023 04:33

MikeMcQuaid merged commit eafdbb3 into Homebrew:master Apr 26, 2023
31 checks passed

dduugg deleted the enable-InvertibleUnlessCondition branch April 26, 2023 05:08

github-actions bot added the outdated PR was locked due to age label May 27, 2023

github-actions bot locked as resolved and limited conversation to collaborators May 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable Style/InvertibleUnlessCondition cop #15265

Enable Style/InvertibleUnlessCondition cop #15265

dduugg commented Apr 18, 2023 •

edited

dduugg Apr 18, 2023

issyl0 Apr 18, 2023

dduugg commented Apr 18, 2023

issyl0 left a comment

issyl0 Apr 18, 2023

issyl0 Apr 18, 2023

dduugg Apr 18, 2023

issyl0 Apr 18, 2023

MikeMcQuaid Apr 19, 2023

issyl0 Apr 18, 2023

dduugg Apr 18, 2023 •

edited

issyl0 Apr 18, 2023

issyl0 Apr 18, 2023

dduugg Apr 18, 2023

issyl0 Apr 18, 2023

MikeMcQuaid Apr 19, 2023

issyl0 Apr 18, 2023

MikeMcQuaid left a comment

MikeMcQuaid commented Apr 19, 2023 •

edited

issyl0 commented Apr 19, 2023

reitermarkus Apr 20, 2023

reitermarkus Apr 20, 2023

dduugg commented Apr 20, 2023

MikeMcQuaid commented Apr 21, 2023

issyl0 commented Apr 21, 2023

dduugg commented Apr 22, 2023

		return if count <= 1

		add_error "only a single postflight stanza is allowed"

Enable Style/InvertibleUnlessCondition cop #15265

Enable Style/InvertibleUnlessCondition cop #15265

Conversation

dduugg commented Apr 18, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dduugg commented Apr 18, 2023

issyl0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dduugg Apr 18, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid left a comment

Choose a reason for hiding this comment

MikeMcQuaid commented Apr 19, 2023 • edited

issyl0 commented Apr 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dduugg commented Apr 20, 2023

MikeMcQuaid commented Apr 21, 2023

issyl0 commented Apr 21, 2023

dduugg commented Apr 22, 2023

dduugg commented Apr 18, 2023 •

edited

dduugg Apr 18, 2023 •

edited

MikeMcQuaid commented Apr 19, 2023 •

edited