Rubocop v0.87+ compatibility #179

marcandre · 2020-07-06T17:29:16Z

Rubocop 0.87 is a prelude to 1.0 and introduces major refactoring of internal classes, some of which are used by erb-lint.

This PR maintains compatibility.

Note that this needs the very latest parser to use the new TreeRewriter#import feature.

* Pegs simplecov * Pegs Blacklight to no greater than 7.5.0 * UPDATE gems * rubocop 0.87.1 → 0.86.0 [changelog](https://github.com/rubocop-hq/rubocop/blob/master/CHANGELOG.md#0860-2020-06-22) * Had to downgrade rubocop due to Shopify/erb-lint#179

ecylmz · 2020-07-20T07:59:19Z

Hi,

It's been almost 2 weeks. Why is PR not merged?

exterm · 2020-07-30T19:26:11Z

Thank you for your contribution, and sorry for not responding earlier. Shopify is dramatically cutting back on the use of erb, so maintaining erblint is not a top priority right now. We do still use it though, welcome your contribution, and will get to work reviewing it right now!

exterm · 2020-07-30T19:29:31Z

lib/erb_lint/corrector.rb

    end

-    def diagnostics
-      corrector.diagnostics
+    if defined?(::RuboCop::Cop::Base)


I'm not sure I understand what's going on here. Are you using the existence of ::RuboCop::Cop::Base to deduce whether ::RuboCop::Cop::Legacy::Corrector exists?

If that's the case, would it be an option to just check for the existence of rubocop/cop/legacy/corrector?

Are you using the existence of ::RuboCop::Cop::Base to deduce whether ::RuboCop::Cop::Legacy::Corrector exists?

Yes

would it be an option to just check for the existence of rubocop/cop/legacy/corrector?

Yes, but I'm not sure I understand how you are proposing to do that exactly, or the benefits.

Actually, I'm using it to deduce if ::RuboCop::Cop::Legacy::Corrector exists and which API to use...

I'm trying to find out what the actual desired semantics are. As this condition is repeated throughout the diff and you don't seem to be using ::RuboCop::Cop::Base directly anywhere, I guess you're looking for whether or not the rubocop version used is 0.87+?

Yes, a check on version number would be equivalent

I think I'd rather have a check on the version number as that means it'd be more obvious when to remove the added code paths.

But I'm also interested in hearing what @thegedge thinks about this.

it'd be more obvious when to remove the added code paths.

That's a good point 👍

Agreed, a version check would definitely make this a little clearer in terms of when we should remove.

Ideally, we would better abstract away the details of Rubocop, but that's outside the scope of this PR. One thing this PR highlights is all the coupling points 🙂

Ok, let me switch to version check then.

exterm · 2020-07-30T19:31:23Z

lib/erb_lint/linters/hard_coded_string.rb

@@ -63,7 +63,7 @@ def autocorrect(processed_source, offense)
        string = offense.source_range.source
        return unless (klass = load_corrector)
        return unless string.strip.length > 1
-        node = RuboCop::AST::StrNode.new(:str, [string])
+        node = ::RuboCop::AST::StrNode.new(:str, [string])


What's the reason for explicitly fully qualifying the Rubocop references?

The first thing I wanted to check in the code was all dependencies on RuboCop. Given that erb_lint has a class called Rubocop (no capital 'C' though), this wasn't perfectly clear at first so I thought that it was a good idea to do this, instead of relying on the context or how the letter 'c' is capitalized.

I'm 👍 with fully qualifying external references (in general), but especially if there's a similarly named thing internally.

exterm · 2020-07-30T19:45:23Z

Fair warning - Shopify is not working Fridays right now and I'm running out of time for today, I'll get back to you next week.

erb_lint.gemspec

exterm · 2020-08-04T19:37:34Z

lib/erb_lint/linters/rubocop.rb

+        activate_team(processed_source, source, offset, code_node, build_team)
+      end
+
+      if ::RuboCop::Version::String.to_f >= 0.87


Why do these conditions on a class level, instead of inside the method body?

Is this just to avoid repeated .to_f >= 0.87 at runtime?

Yes. More efficient, and I find it cleaner this way; it makes it clear what actions need be different according to the version.

I was actually thinking the opposite, because your version duplicates some code, instead of just putting the conditionals around the code that is actually different.

But I don't have a strong opinion on this; either way it's clear what to remove.

exterm · 2020-08-04T19:38:42Z

Thank you for structuring your changes into separate commits, makes the review so much easier 👍

exterm · 2020-08-04T19:51:45Z

lib/erb_lint/corrector.rb

    end

-    def diagnostics
-      corrector.diagnostics
+    if ::RuboCop::Version::String.to_f >= 0.87


Looking at the tests, it seems that you need to

Suggested change

if ::RuboCop::Version::String.to_f >= 0.87

if ::RuboCop::Version::STRING.to_f >= 0.87

exterm · 2020-08-04T19:53:47Z

OK, I'm getting close to a ✅ in my mind 😄

The main thing that I'm concerned with at this point is that we're not testing both code paths. I wonder if we could configure travis to do separate builds for old-rubocop vs new-rubocop.

If that leads to too many build variations we may want to think about dropping Ruby 2.4.1

thegedge · 2020-08-04T20:02:50Z

If that leads to too many build variations we may want to think about dropping Ruby 2.4.1

We should probably do that anyways, since Ruby 2.4 is EOL (and 2.5 is soon to be EOL).

marcandre · 2020-08-04T20:14:15Z

If that leads to too many build variations we may want to think about dropping Ruby 2.4.1

We should probably do that anyways, since Ruby 2.4 is EOL (and 2.5 is soon to be EOL).

I didn't make changes to the .travis.yml file, but yes, it is needed. I'd recommend including ruby-head too (allowed failures if need be)

exterm · 2020-08-04T21:13:01Z

working on a setup to test both code paths in #181

exterm · 2020-08-04T21:21:07Z

I'd love to get the CI setup figured out first, cherry-pick it over here and merge both together

thegedge · 2020-08-04T21:25:18Z

Yeah, I'd like to see the ✅ for both the old and new Rubocop, but otherwise everything looks good to me. Thanks for your contribution, @marcandre. Sorry it took so long for us get to it 😓

We want to test code paths specific to these new APIs while still also testing the old code paths for compatibility

marcandre · 2020-08-05T13:34:09Z

Cherry-picked, all green.

exterm

🎉

Thank you so much for your contribution, and the persistence!

marcandre force-pushed the rubocop_v1 branch from 9f58d40 to 2582f43 Compare July 6, 2020 19:38

tomhughes mentioned this pull request Jul 7, 2020

Bump rubocop from 0.86.0 to 0.87.0 openstreetmap/openstreetmap-website#2692

Closed

darrenterhune mentioned this pull request Jul 8, 2020

Undefined method reject for Rubocop::Cop::Registry in Rubocop::Cop::Team#roundup_relevant_cops rubocop/rubocop#8077

Closed

marcandre force-pushed the rubocop_v1 branch from 2582f43 to 3714f79 Compare July 8, 2020 17:58

murny mentioned this pull request Jul 9, 2020

Bump rubocop from 0.86.0 to 0.87.1 ualbertalib/jupiter#1749

Closed

cdmo added a commit to psu-libraries/psulib_blacklight that referenced this pull request Jul 9, 2020

Had to downgrade rubocop due to Shopify/erb-lint#179

fbc4666

tomhughes mentioned this pull request Jul 9, 2020

Error with rubocop 0.85 #174

Closed

gravitystorm mentioned this pull request Jul 9, 2020

Bump rubocop from 0.86.0 to 0.87.1 openstreetmap/openstreetmap-website#2694

Closed

gravitystorm mentioned this pull request Jul 15, 2020

Bump rubocop from 0.86.0 to 0.88.0 openstreetmap/openstreetmap-website#2706

Closed

ecylmz mentioned this pull request Jul 20, 2020

Zeitwerk uyarılarını düzelt ve RuboCop'u Güncelle omu/nokul#1923

Merged

9 tasks

exterm requested review from thegedge and exterm July 30, 2020 19:22

exterm reviewed Jul 30, 2020

View reviewed changes

thegedge reviewed Aug 4, 2020

View reviewed changes

erb_lint.gemspec Show resolved Hide resolved

marcandre force-pushed the rubocop_v1 branch from 3714f79 to 20b0ac4 Compare August 4, 2020 16:41

exterm reviewed Aug 4, 2020

View reviewed changes

marcandre force-pushed the rubocop_v1 branch from 20b0ac4 to 4aedf4e Compare August 4, 2020 20:12

exterm mentioned this pull request Aug 4, 2020

separate test run for rubocop >= 0.87 #181

Closed

exterm mentioned this pull request Aug 4, 2020

Drop support for Ruby 2.4 #182

Merged

Philip Müller and others added 4 commits August 4, 2020 19:56

rubocop introduces pre-1.0 APIs in 0.87

930378b

We want to test code paths specific to these new APIs while still also testing the old code paths for compatibility

Be explicit with usage of ::RuboCop classes

a5927f4

Avoid redundant recalculations

87b62d8

Update for RuboCop v0.87+

f3b6b0a

marcandre force-pushed the rubocop_v1 branch from 4aedf4e to f3b6b0a Compare August 4, 2020 23:56

thegedge approved these changes Aug 5, 2020

View reviewed changes

exterm approved these changes Aug 5, 2020

View reviewed changes

exterm merged commit 73c9a6e into Shopify:master Aug 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rubocop v0.87+ compatibility #179

Rubocop v0.87+ compatibility #179

marcandre commented Jul 6, 2020

ecylmz commented Jul 20, 2020

exterm commented Jul 30, 2020

exterm Jul 30, 2020 •

edited

marcandre Jul 30, 2020

marcandre Jul 30, 2020

exterm Jul 30, 2020

marcandre Jul 30, 2020

exterm Jul 30, 2020

marcandre Jul 30, 2020

thegedge Aug 4, 2020

marcandre Aug 4, 2020

marcandre Aug 4, 2020

exterm Jul 30, 2020

marcandre Jul 30, 2020

thegedge Aug 4, 2020

exterm commented Jul 30, 2020 •

edited

exterm Aug 4, 2020

marcandre Aug 4, 2020

exterm Aug 4, 2020

exterm commented Aug 4, 2020

exterm Aug 4, 2020

exterm commented Aug 4, 2020

thegedge commented Aug 4, 2020

marcandre commented Aug 4, 2020

exterm commented Aug 4, 2020

exterm commented Aug 4, 2020

thegedge commented Aug 4, 2020

marcandre commented Aug 5, 2020

exterm left a comment

	if ::RuboCop::Version::String.to_f >= 0.87
	if ::RuboCop::Version::STRING.to_f >= 0.87

Rubocop v0.87+ compatibility #179

Rubocop v0.87+ compatibility #179

Conversation

marcandre commented Jul 6, 2020

ecylmz commented Jul 20, 2020

exterm commented Jul 30, 2020

exterm Jul 30, 2020 • edited

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

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

exterm commented Jul 30, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exterm commented Aug 4, 2020

Choose a reason for hiding this comment

exterm commented Aug 4, 2020

thegedge commented Aug 4, 2020

marcandre commented Aug 4, 2020

exterm commented Aug 4, 2020

exterm commented Aug 4, 2020

thegedge commented Aug 4, 2020

marcandre commented Aug 5, 2020

exterm left a comment

Choose a reason for hiding this comment

exterm Jul 30, 2020 •

edited

exterm commented Jul 30, 2020 •

edited