Bump to latest rubocop 1.16.0 #102

sagotsky · 2021-06-04T13:18:23Z

Fixes #101

What did we change?

Bumped rubocop to most recent version.
Bumped rubocop-{rails,rspec} as well
Disabled a large number of new cops 872b163
Marked this as a new major version

Why are we doing this?

Application fitness 🏋️
After 1.0, new cops will be introduced in a default state. This will make future version bumps much easier.
Disabling many of those cops is likely going to be my spicy take of the day. Bluntly speaking, we should have been doing more minor version bumps along the way. That would have introduced those cops in pending states and then phased them in to an enabled state. I suppose we could retroactively step through all the rubocop version bumps, but I'd rather just fast forward past that step.
We've upgraded rubocop major versions. They had breaking changes. Therefore we inherited their breaking changes.

How was it tested?

Specs
Locally

sagotsky · 2021-06-04T13:21:09Z

~~Looks like there's no CI for this repo. I get failures on the following specs locally.~~

~~rspec ./spec/rubocop/cop/ezcater/graphql_fields_naming_spec.rb ~~ ~~rspec ./spec/rubocop/cop/ezcater/style_dig_spec.rb`~~

All green!

It looks like `check_name` now knows how to call `.loc.expression` on an arugment. We were doing that on our own as well. Cleaned that up.

This gem requires 2.6+. The 2.3 in this spec is no longer supported and so it would not run.

jaeger401

Gotta love a +3/-3 that adds this much value. Thanks for taking this on!

jaeger401 · 2021-06-04T14:13:47Z

Probably worth updating the PR description to "Fixes #101" so merging this closes the issue automagically?

sagotsky · 2021-06-04T14:15:20Z

Good call! I forgot how much I miss that kind of github magic.

jmpage

Thoughts on testing this a prerelease of this version against a repo to get an idea of what might break with this upgrade?

Thoughts on updating CHANGELOG.md and the VERSION, as appropriate?

sagotsky · 2021-06-04T14:40:37Z

@jmpage Yes, to all.

sagotsky · 2021-06-04T14:41:19Z

The prerelease testing is what blocked a new version number. I'm not sure just how breaking this is yet.

Rails specific cops migrated out of rubocop and into rubocop-rails. Our rubocop-rails gem was behind and so it was missing those cops.

Our old code was reaching into private APIs. Lo and behold, the private APIs gated off access with `private_constant` and then renamed their methods. Found the new way to access this method by including the rspec langauge module.

jmpage

Thoughts on testing this on a non-ez-rails app as well, just to see how disruptive it might be?

jmpage · 2021-06-08T15:39:08Z

conf/rubocop.yml

+  Enabled: false
+
+Rails/ArelStar:
+  Enabled: false
+
+Rails/Pick:
+  Enabled: false
+
+Rails/RedundantForeignKey:
+  Enabled: false
+
+RSpec/Capybara/CurrentPathExpectation:
+  Enabled: false
+
+RSpec/Capybara/FeatureMethods:
+  Enabled: false
+
+RSpec/Capybara/VisibilityMatcher:
+  Enabled: false
+
+RSpec/EmptyHook:
+  Enabled: false
+
+RSpec/FactoryBot/AttributeDefinedStatically:
+  Enabled: false
+
+RSpec/FactoryBot/CreateList:
+  Enabled: false
+
+RSpec/FactoryBot/FactoryClassName:
+  Enabled: false
+
+RSpec/MultipleMemoizedHelpers:
+  Enabled: false
+
+RSpec/NotToNot:
+  Enabled: false
+
+RSpec/Rails/HttpStatus:
+  Enabled: false
+
+RSpec/RepeatedIncludeExample:
+  Enabled: false
+
+RSpec/StubbedMock:
+  Enabled: false
+
+RSpec/VariableDefinition:
+  Enabled: false
+
+RSpec/VariableName:
+  Enabled: false
+
+Style/AccessorGrouping:
+  Enabled: false
+
+Style/BisectedAttrAccessor:
+  Enabled: false
+
+Style/CaseLikeIf:
+  Enabled: false
+
+Style/CombinableLoops:
+  Enabled: false
+
+Style/GlobalStdStream:
+  Enabled: false
+
+Style/HashAsLastArrayItem:
+  Enabled: false
+
+Style/HashLikeCase:
+  Enabled: false
+
+Style/KeywordParametersOrder:
+  Enabled: false
+
+Style/OptionalBooleanParameter:
+  Enabled: false
+
+Style/RedundantAssignment:
+  Enabled: false
+
+Style/RedundantRegexpCharacterClass:
+  Enabled: false
+
+Style/RedundantRegexpEscape:
+  Enabled: false
+
+Style/RedundantSelfAssignment:
+  Enabled: false
+
+Style/SingleArgumentDig:
+  Enabled: false
+
+Style/SlicingWithRange:
+  Enabled: false
+
+Style/SoleNestedConditional:
+  Enabled: false


Thoughts on opening an issue on this project for evaluating these for inclusion and include a checkbox TODO list in the description containing the name of each disabled cop?

I definitely want to go through each of these and talk about the interesting ones. GH issues seems like a perfectly fine place to do so.

I figure that GH issues let's us punt the issue of figuring out what to turn on from this PR while leaving us with a reminder to get back to it (which probably requires some deliberation for each cop).

#104

I think it's a good home for repos that don't have jira projects.

jmpage · 2021-06-08T15:40:24Z

spec/rubocop/cop/ezcater/style_dig_spec.rb

@@ -4,7 +4,7 @@
 RSpec.describe RuboCop::Cop::Ezcater::StyleDig, :config do
  subject(:cop) { described_class.new(config) }

-  let(:ruby_version) { 2.3 }
+  let(:ruby_version) { 2.6 }


What prompted this change?

Not gonna lie, I couldn't completely figure this one out. At least not without spending more time on it.

What I think is going on is that this cop had a minimum ruby version that it applied to. The test was making assertions about the style given a particular version. ruby_version isn't mentioned anywhere else in the file, so I assume it's provided by rubocop, but that's where I stopped digging in.

I figured that since the gem required 2.6 or higher, we'd never be looking at a ruby lower than that. Bumping the version here fixed the spec.

jmpage · 2021-06-08T15:41:19Z

CHANGELOG.md

+- Upgrade rubocop:       1.16.0
+- Upgrade rubocop-rails: 2.10.1
+- Upgrade rubocop-rspec: 2.3.0
+- This is a major upgrade because a large number of cops have been introduced, tweaked, or renamed.


Thoughts on including (or linking to) a list of renamed cops so that people can easily update their rubocop:disable statements?

Sure. Is the changelog the appropriate home for that?

It's the thing that dependabot will excerpt in a bump PR and the first place someone should look when determining changes. If the list is short, it's probably fine to include it. If it's longer, it may make sense to create a separate file in a docs/migration_guides/ folder and link to that, or link to an external resource that lives in the rubocop repos.

I've rolled up the breaking changes from the three dependencies into our changelog.

CHANGELOG.md

sagotsky · 2021-06-08T15:50:07Z

@jmpage I'd be game for testing on other repos. Do you have any in mind? If not I'll just run this against whatever is checked out locally.

jmpage · 2021-06-08T20:19:46Z

@sagotsky authentication-rails, identity-rails or ezcater-email-service are reasonably "simple" repos that you could test it against.

sagotsky · 2021-06-08T20:54:34Z

Tested against a handful of repos. All had single digit offenses. RequiredRubyVersion needed to be fixed everywhere (it was the only offense for coupons-rails, ezcater-identity, and authentication-rails`. club-soda-rails was the biggest repo and that weight in at 8 offenses, all of which were redundant. I'm comfortable releasing like this.

jmpage · 2021-06-08T22:12:14Z

🎉

Thank you @sagotsky!

Bump to latest rubocop 1.16.0

dee458e

sagotsky requested a review from a team as a code owner June 4, 2021 13:18

sagotsky added 2 commits June 4, 2021 09:58

Fix GraphqlFieldsNaming cop

b0be550

It looks like `check_name` now knows how to call `.loc.expression` on an arugment. We were doing that on our own as well. Cleaned that up.

Bump ruby version in test to minimum supported by gem

76afd5a

This gem requires 2.6+. The 2.3 in this spec is no longer supported and so it would not run.

jaeger401 approved these changes Jun 4, 2021

View reviewed changes

jmpage reviewed Jun 4, 2021

View reviewed changes

sagotsky added 3 commits June 7, 2021 09:23

Bump rubocop-rails as well

f03fed8

Rails specific cops migrated out of rubocop and into rubocop-rails. Our rubocop-rails gem was behind and so it was missing those cops.

Bump rubocop-rspec too

9327c0a

Fix RspecDotNotSelfDot

8cce4ea

Our old code was reaching into private APIs. Lo and behold, the private APIs gated off access with `private_constant` and then renamed their methods. Found the new way to access this method by including the rspec langauge module.

jmpage reviewed Jun 8, 2021

View reviewed changes

sagotsky mentioned this pull request Jun 8, 2021

Double check disabled cops from last major upgrade #104

Open

60 tasks

sagotsky force-pushed the js/rubocop-1.16.0 branch 2 times, most recently from 05dcb5f to ada7d38 Compare June 8, 2021 20:16

sagotsky added 2 commits June 8, 2021 16:17

Disable new cops

fa1f18c

Version 3.0.0

ada7d38

jmpage approved these changes Jun 8, 2021

View reviewed changes

sagotsky merged commit cea1508 into master Jun 9, 2021

sagotsky deleted the js/rubocop-1.16.0 branch June 9, 2021 14:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump to latest rubocop 1.16.0 #102

Bump to latest rubocop 1.16.0 #102

sagotsky commented Jun 4, 2021 •

edited

Loading

sagotsky commented Jun 4, 2021 •

edited

Loading

jaeger401 left a comment

jaeger401 commented Jun 4, 2021

sagotsky commented Jun 4, 2021

jmpage left a comment

sagotsky commented Jun 4, 2021

sagotsky commented Jun 4, 2021

jmpage left a comment

jmpage Jun 8, 2021 •

edited

Loading

sagotsky Jun 8, 2021

jmpage Jun 8, 2021

sagotsky Jun 8, 2021

jmpage Jun 8, 2021

sagotsky Jun 8, 2021

jmpage Jun 8, 2021

sagotsky Jun 8, 2021

jmpage Jun 8, 2021 •

edited

Loading

sagotsky Jun 8, 2021

sagotsky commented Jun 8, 2021

jmpage commented Jun 8, 2021

sagotsky commented Jun 8, 2021

jmpage commented Jun 8, 2021

Bump to latest rubocop 1.16.0 #102

Bump to latest rubocop 1.16.0 #102

Conversation

sagotsky commented Jun 4, 2021 • edited Loading

What did we change?

Why are we doing this?

How was it tested?

sagotsky commented Jun 4, 2021 • edited Loading

jaeger401 left a comment

Choose a reason for hiding this comment

jaeger401 commented Jun 4, 2021

sagotsky commented Jun 4, 2021

jmpage left a comment

Choose a reason for hiding this comment

sagotsky commented Jun 4, 2021

sagotsky commented Jun 4, 2021

jmpage left a comment

Choose a reason for hiding this comment

jmpage Jun 8, 2021 • edited Loading

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

Choose a reason for hiding this comment

jmpage Jun 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sagotsky commented Jun 8, 2021

jmpage commented Jun 8, 2021

sagotsky commented Jun 8, 2021

jmpage commented Jun 8, 2021

sagotsky commented Jun 4, 2021 •

edited

Loading

sagotsky commented Jun 4, 2021 •

edited

Loading

jmpage Jun 8, 2021 •

edited

Loading

jmpage Jun 8, 2021 •

edited

Loading