-
Notifications
You must be signed in to change notification settings - Fork 246
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
Issue #504: try merging invoca .rubocop.yml #508
Issue #504: try merging invoca .rubocop.yml #508
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some files could not be reviewed due to errors:
.rubocop.yml: Layout/EmptyLineAfterGuardClause has the wrong namespace - shou...
.rubocop.yml: Layout/EmptyLineAfterGuardClause has the wrong namespace - should be Style Warning: unrecognized cop Lint/SuppressedException found in .rubocop.yml Warning: unrecognized cop Layout/HashAlignment found in .rubocop.yml Warning: unrecognized cop Layout/HeredocIndentation found in .rubocop.yml Warning: unrecognized cop Lint/RedundantRequireStatement found in .rubocop.yml Warning: unrecognized cop Naming/RescuedExceptionsVariableName found in .rubocop.yml Error: Unknown Ruby version 2.6 found in `TargetRubyVersion` parameter (in .rubocop.yml). Supported versions: 2.1, 2.2, 2.3, 2.4, 2.5
@KapilSachdev What do you think? |
@@ -34,6 +34,6 @@ group :development do | |||
gem 'netrc', require: false | |||
gem 'octokit', require: false | |||
gem 'pry-rescue' | |||
gem 'rubocop', '~> 0.49.0' # TODO: should match Gemfile HoundCi | |||
gem 'rubocop', '0.91.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can update to 1.2.0
, I think there will be not much changes as I have checked in my local machine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, the highest supported by hound is 0.91.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use something else than hound in that case? Github actions like lint-action.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KapilSachdev That sounds reasonable. So that we don't bog this Issue down, can you file a separate issue for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The time i commented about lint-action
, i was unaware of one issue that they have i.e. annotations don't work when PR is created from a fork. Which is where most PRs come from. Though there is a new pull_request_target
event for that, but it still isn't working (from what i had tried).
shoulda-matcher
gem also recently reverted from lint-action
(which initially uses hound) and has fallen back to non-annotation based rubocop
linting (like what rails uses).
I will create a issue for rubocop
linting based on github actions.
spec/support/acceptance_helper.rb
Outdated
@listener = if callback | ||
Listen.send(*args) do |modified, added, removed| | ||
# Add changes to trigger frozen Hash error, making sure lag is enough | ||
_add_changes(:modified, modified, changes) | ||
_add_changes(:added, added, changes) | ||
_add_changes(:removed, removed, changes) | ||
|
||
unless callback == :track_changes | ||
callback.call(modified, added, removed) | ||
end | ||
end | ||
else | ||
Listen.send(*args) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding the assignment in the next line could help in less indentation spaces.
@listener = if callback | |
Listen.send(*args) do |modified, added, removed| | |
# Add changes to trigger frozen Hash error, making sure lag is enough | |
_add_changes(:modified, modified, changes) | |
_add_changes(:added, added, changes) | |
_add_changes(:removed, removed, changes) | |
unless callback == :track_changes | |
callback.call(modified, added, removed) | |
end | |
end | |
else | |
Listen.send(*args) | |
end | |
@listener = | |
if callback | |
Listen.send(*args) do |modified, added, removed| | |
# Add changes to trigger frozen Hash error, making sure lag is enough | |
_add_changes(:modified, modified, changes) | |
_add_changes(:added, added, changes) | |
_add_changes(:removed, removed, changes) | |
unless callback == :track_changes | |
callback.call(modified, added, removed) | |
end | |
end | |
else | |
Listen.send(*args) | |
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also agree, this alignment change is a mess, and it should probably just be
@listener = if callback
Listen.send(...)
else
...
end
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rubocop wouldn't tolerate that formatting. I put 10 minutes into finding an option that would, but I didn't find it. Instead, I tried the if
on the next line and it was ok with that. Of course then it complained that the method was too long! So I moved an unless
from leading to trailing. 🤦
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think below should help. However as you have used next line, you won't need this then. Next lines looks good to me.
Layout/EndAlignment:
EnforcedStyleAlignWith: variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @KapilSachdev. I made that EnforcedStyleAlignWith: variable
change and put the @listener =
code back the way it was, then ran rubocop -a -A
. e1045d2
…hat don't fit well with code in this repo
cfdd1f1
to
a6b6b5d
Compare
@KapilSachdev I merged up to latest master and applied all the changes in discussion here.
|
ad57854
to
c9a6205
Compare
…h a few disables
39a2a4c
to
b2fbfaf
Compare
…iable; move LineLength to Layout namespace
I made an attempt to address issue #504 :
rubocop -a
: 931106frubocop -A
: 8c5fed6After the above tuning and auto-correcting, there were 15 remaining offenses:
After fixing the easy ones, there are 5 offenses left: