-
Notifications
You must be signed in to change notification settings - Fork 761
Upgrade to latest rubocop packages #178
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
Conversation
Travis will fail because this change drops support for ruby 2.3 |
@@ -68,10 +68,6 @@ Lint/EmptyInterpolation: | |||
Lint/EmptyWhen: | |||
Enabled: false | |||
|
|||
Lint/EndInMethod: |
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 removed this cop
@@ -155,10 +154,6 @@ RSpec/InstanceVariable: | |||
Description: 'Checks for the usage of instance variables.' | |||
Enabled: false | |||
|
|||
RSpec/InvalidPredicateMatcher: |
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 removed this cop
].join("\n") | ||
|
||
inspect_source(source) | ||
expect(cop.offenses.size).to eq(1) |
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.
FLAG: This test verifies that an error was raised, but the spec text says "allows".
@bmulholland thanks for contributing. I just started working on #181 which unfortunately duplicates some of your work here. I wasn't comfortable jumping all the way to rubocop 1.x in one go, so I upgraded to 0.93.1 (the last non-1.x rubocop available). Perhaps we can do this in a two-phase update to minimize the required changes for consumers of this gem? |
@bmulholland this has been incorporated into #189. This, and a few other changes, have been incorporated in our newest v5.0.0 gem release which is now available on rubygems. Thank you for your contribution, and apologies for the long delay. cc also @maknz @kelvinlzhang @blueberrystream @bj-mcduck @kkohrt who left emoji reactions on the PR and thus presumably may still be interested in seeing this get over the finish line |
I know this is perhaps not a priority for Airbnb, and likely a project in and of itself to adopt internally, but the cops in this project are super useful for some of us externally. Unfortunately for us non-Airbnbs, the super-tight and now a little out of date packages mean that it's difficult to use alongside other packages, and especially tricky for adopting in CodeClimate. It makes it hard to use this alongside e.g. rubocop-sorbet.
So I upgraded all the packages to the latest, and allowed any minor version to prevent things from getting so out of date again. The new rspec format took some time, but is IMO much cleaner and more specific. Do note that there's a way to truncate the strings in the expectation blocks by cutting off the /end/, but the
[...]
syntax doesn't work in the beginning or middle of a message. For that reason I've had to include the full text of the message if there's something at the end of the cop output that we want to check.Even if Airbnb doesn't adopt the latest version of the cops, would you be open to accepting this PR and releasing a new gem version? Alternatively, I suppose I could fork it and issue my own gem, but I don't want to bear /all/ the maintenance burden :)
And either way, check out the one TODO I marked in a spec where the spec didn't match the tested behaviour - what's actually intended?