Skip to content

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

Closed
wants to merge 8 commits into from
Closed

Upgrade to latest rubocop packages #178

wants to merge 8 commits into from

Conversation

bmulholland
Copy link

@bmulholland bmulholland commented Dec 2, 2020

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?

@bmulholland
Copy link
Author

Travis will fail because this change drops support for ruby 2.3

@@ -68,10 +68,6 @@ Lint/EmptyInterpolation:
Lint/EmptyWhen:
Enabled: false

Lint/EndInMethod:
Copy link
Author

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:
Copy link
Author

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)
Copy link
Author

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".

@pariser
Copy link
Contributor

pariser commented Apr 8, 2021

@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?

@pariser pariser mentioned this pull request Apr 8, 2021
@RenzoMinelli RenzoMinelli mentioned this pull request Feb 1, 2023
@pariser
Copy link
Contributor

pariser commented Feb 13, 2023

@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

@pariser pariser closed this Feb 13, 2023
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.

2 participants