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

Fix syntax for rubocop #194

Closed
wants to merge 1 commit into from
Closed

Fix syntax for rubocop #194

wants to merge 1 commit into from

Conversation

jhunschejones
Copy link

@jhunschejones jhunschejones commented Oct 10, 2021

Description:

@thomasfl / @AlexWayfer CI started failing in September for a rubocop formatting offense:
From Cirrus Logs

bundle exec rubocop
Inspecting 15 files
........W......
Offenses:
lib/filewatcher/spec_helper/watch_run.rb:16:16: W: [Correctable] Lint/ElseLayout: Odd else layout detected. Did you mean to use elsif?
          else File.join(TMP_DIR, filename)
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
15 files inspected, 1 offense detected, 1 offense auto-correctable

This PR just makes a small tweak to the formatting of the line in question to make rubocop happy again:

bundle exec rubocop
Inspecting 15 files
...............

15 files inspected, no offenses detected

Testing:

All tests should continue to pass

@AlexWayfer
Copy link
Member

Thank you for the patch!

But… there are 2 moments:

  1. I'd like to enforce RuboCop version lock to the patch version for receiving new cops (enabled by our config) only in Depfu-bot PRs (or similar) with separated CI runs.
  2. An easy search forwards me to the RuboCop's issue: Fix Lint/ElseLayout when using multi-branch statements rubocop/rubocop#10147
    And I see a kind of consensus there that this (Lint/*Layout, LOL) cop should check consistency of if-else blocks styles (and account if-then + one-line else) instead of just simple "ouh, there is one-line else, this is wrong!".

I like this code style, and until RuboCop (surely) forces to another (multi-line) — I wouldn't like to change.

(BTW you can create a small PR for the first point with a lock to the latest successful version, like 1.21, as I see)
(never mind: I've already pushed this to the master 30b41b7 while inspecting additional details and discussions)

@jhunschejones
Copy link
Author

Awesome, sounds like the build is fixed then! I'll close out this PR.

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

Successfully merging this pull request may close these issues.

None yet

2 participants