Update all Rubocop dependencies

[csutter]

A reasonably major change so I've bumped the major version number. Rubocop and a bunch of its sub-gems have passed 1.0 now, so their versioning should be more stable, so I've made the gemspec more lenient w.r.t. patch versions (I was going to be super ambitious and even include minor versions but reckoned you'd probably like to keep things relatively predictable).

I reckon some of the new cops may be undesirable so there may need to be further updates once a few apps update to this version.

• Update rubocop to 1.7.0
• Update rubocop-ast to 1.4.0
• Update rubocop-rails to 2.9.1
• Update rubocop-rspec to 2.1.0
• Make stable dependencies (>= 1.0) less strict on patch version
• Explicitly set target Ruby version to >= 2.7 in gemspec and Rubocop
  TargetRubyVersion setting
• Fix namespace change of Capybara/FeatureMethods
• Update gem version to 4.0 due to major changes

[kevindew]

Thanks very much for this @csutter 🎉 - It does look like we've fallen rather far behind. Further than I had realised. So it's great you've got the ball rolling on this 👍

I've been trying this out on some of our repos and it is flagging quite a lot of violations which is a bit of a pain and may make this a bit difficult to transition to. Some of these look good and autocorrectable (Style/StringConcatenation, Layout/EmptyLinesAroundAttributeAccessor) others seem like they might be a bit of a pain (Naming/VariableNumber, Lint/MissingSuper). I've put some details up about them below so I can share the impact with other GOV.UK devs.

A thing I noticed is that when we run rubocop in repos with minitest we're now seeing:

Tip: Based on detected gems, the following RuboCop extension libraries might be helpful:
  * rubocop-minitest (http://github.com/rubocop-hq/rubocop-minitest)

You can opt out of this message by adding the following to your config (see https://docs.rubocop.org/rubocop/extensions.html#extension-suggestions for more options):
  AllCops:
    SuggestExtensions: false

I feel like we should set SuggestExtensions: false in this gem as this bundles all our various conventions together. It seems a bit of an unhelpful message for consumers of this.

---

Analysis of rules that might be a pain and thoughts:

Naming/VariableNumber: Use normalcase for symbol numbers. (https://rubystyle.guide#snake-case-symbols-methods-vars-with-numbers)

This expects rescue_from GdsApi::ContentStore::ItemNotFound, with: :error_404 to be rescue_from GdsApi::ContentStore::ItemNotFound, with: :error404 - which is consistent with our approach to variable names with numbers but I don't think it's particularly liked. This seems the most controversial change (in Smart Answers it has > 200 offences) and suggests changing things like born_on_or_before_6_april_1935 to born_on_or_before_6april1935 which seem much less legible. I wonder if we're fighting against an unwritten GOV.UK convention following https://rubystyle.guide/#snake-case-symbols-methods-vars-with-numbers and should change to snake_case - simplest thing for now would be disable CheckMethodNames and CheckSymbols options for this so we could defer this conversation to be outside this PR.

Style/OptionalBooleanParameter: Use keyword arguments when defining method with boolean argument. (https://rubystyle.guide#boolean-keyword-arguments)

This expects def format_document_data(documents, data_category = "", with_image_url = false) to be def format_document_data(documents, data_category = "", with_image_url: false) - seems good to go in.

Lint/ConstantDefinitionInBlock: Do not define constants this way within a block. (https://rubystyle.guide#no-constant-definition-in-block)

This seems to be mostly when constants are defined in test files or classes. This is arguably a dubious practice where there are better Ruby approaches to do this so I think this is good to go in (but might cause some pain).

Lint/MissingSuper: Call super to initialize state of the parent class. (https://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Lint/MissingSuper)

This seems to cause a pain for classes that are inherited without an initialize method (~100 offences in https://github.com/alphagov/content-publisher) - I'm not too sure if this is overstepping a linting mark here and will annoy people but my gut says it's good to include.

RSpec/StubbedMock: Prefer allow over expect when configuring a response. (https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/StubbedMock)

This disallows expect(x).to receive(y) - this rule should be disabled for consistency with the disabling of RSpec/MessageSpies, otherwise I fear it'll be quite disruptive.

RSpec/MultipleMemoizedHelpers: Example group has too many memoized helpers [6/5] (https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/MultipleMemoizedHelpers)

This limits the amount of let definitions in a Spec. I think this should be disabled for consistency with [the convention of disabling arbitrary limits](RSpec/MultipleMemoizedHelpers: Example group has too many memoized helpers [6/5] (https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/MultipleMemoizedHelpers)

Rails/LexicallyScopedActionFilter: update is not explicitly defined on the class. (https://rails.rubystyle.guide#lexically-scoped-action-filter)

This complains if filters are specified for actions that are defined in inherited actions. Will be a pain for a https://github.com/alphagov/whitehall but rule intent seems good so I think it's good to go in.

Gemspec/RequiredRubyVersion: required_ruby_version should be specified.

This complains if a gem doesn't specify a required_ruby_version or when the .rubocop.yml for a gem references a Ruby version greater than the one in the Gemfile. We (GOV.UK) often don't set a required_ruby_version (example) and often have .ruby-version files that are newer Rubies than what the gem may work with. It strikes me here that GOV.UK isn't following Ruby versioning best practices for gems and this rule would help that so I think this rule would be good to keep, but would come with some pain of adoption.

[kevindew]

It'd be more conventional to flag this as "Unreleased" as per: https://github.com/alphagov/rubocop-govuk/pull/19/files#diff-06572a96a58dc510037d5efa622f9bec8519bc1beab13c9f251e97e657a9d4edR1

[kevindew]

As an aside it looks like we've had points of contention in the past regarding what deems an update to this project to be a major version bump: #54 so that might be something to leave to us to have an internal chat on. I think it probably counts as a major version though given the major bump of rubocop - we tend to prefix the breaking changes with "BREAKING: " - I guess the breaking ones we have are rubocop, rubocop-ast and rubocop-rspec?

[csutter]

Fair – my thought was that "this upgrade will be painful downstream", but from the point of view of this gem's API, it hasn't changed at all. Happy to defer that decision to release time!

[kevindew]

Your logic to apply this to > 1 ones makes sense to me. It wouldn't surprise me too much if we hit a problem and need to go back but it does seem better to go this direction so bug fixes don't need a new gem release.

[csutter]

Agree, I can't imagine it would be too much of an issue. Rubocop versioning has this to say since their move past 1.0:

RuboCop is stable between major versions, both in terms of API and cop configuration. We aim the ease the maintenance of RuboCop extensions (by keeping the API stable) and the upgrades between RuboCop releases (by not enabling new cops and changing the configuration of existing cops). All big (breaking) changes are reserved for major releases.

And:

Bug-fix (point) releases (if any) address only serious bugs and never contain new features.

We already have NewCops: disable in the default config, so any new rules between explicit releases of rubocop-govuk should not impact apps when patch level releases come out.

[kevindew]

Great stuff 👍 Yeah my concern lies from a recent memory that the version of rubocop-ast could cause a bit of a pain hopefully their release process will mean that no longer happens at patch versions.

[kevindew]

Thanks @csutter this looks good to me. I'll try chat to some other people about this over the next couple of days. It seems reasonable to me that we merge this and look at resolving our rule amends in a separate PR once merged, then you'll not get caught up in our policies. But I'll see if that concerns anyone else.

[benthorner]

Thanks @csutter for pushing this and @kevindew for getting a handle on the consequences it will have around GOV.UK. We do seem to have a problem keeping up-to-date with mainstream RuboCop.

I don't think it's feasible for us to keep up-to-date with RuboCop on a regular basis. But neither do we want to miss out on specific new features. Perhaps a compromise here would be to switch to an "allowed list" approach, which would minimise future disruption. I think the most important thing about a styleguide is that it's consistent and stable.

Anyway, let's see if we can get this in. I've tested this with a bunch of apps: Email Alert Frontend, Email Alert API, Smart Answers, Whitehall, Content Publisher and Collections Publisher.

I've also tried to go through the huge CHANGELOG. Around half of the new Cops are either pending or disabled. The following are enabled, but appear to be benign (from my testing):

• Lint/DuplicateElsifCondition (not seen, makes sense)
• Style/HashAsLastArrayItem (not seen, autocorrectable)
• Lint/DuplicateRescueException (not seen, makes sense)
• Lint/FloatComparison (not seen, makes sense)
• Lint/TopLevelReturnWithArgument (not seen)
• Lint/EmptyConditionalBody (not seen, makes sense)
• Lint/OutOfRangeRegexpRef (not seen, makes sense)
• Style/RedundantSelfAssignment (not seen, makes sense)
• Layout/EmptyLineAfterMultilineCondition (not seen)
• Style/SoleNestedConditional (autocorrectable)
• Style/KeywordParametersOrder (autocorrectable)
• Lint/EmptyFile (not seen, makes sense)
• Lint/TrailingCommaInAttributeDeclaration (autocorrectable)
• Layout/BeginEndAlignment (not seen)
• Lint/IdentityComparison (autocorrectable)
• Lint/HashCompareByIdentity (not seen, makes sense)
• Style/ClassEqualityComparison (autocorrectable)
• Lint/RedundantSafeNavigation (not seen, makes sense)
• Lint/UselessTimes (not seen, makes sense)

I have seen the following, all of which seem good to go in:

• Style/OptionalBooleanParameter (lots of these, makes sense)
• Lint/ConstantDefinitionInBlock (seen a few, makes sense)
• Style/ExplicitBlockArgument (a few of these, autocorrectable)
• Style/GlobalStdStream (seen a few, makes sense)
• Lint/BinaryOperatorWithIdenticalOperands (seen in Smart Answers, makes sense)
• Lint/SelfAssignment (seen in Whitehall, makes sense)
• Lint/DuplicateRequire (seen in Whitehall, makes sense)
• Style/CaseLikeIf (lots of these, unsafe autocorrectable)
• Lint/UnreachableLoop (not seen, makes sense)

I'm less convinced about the following:

• Style/CombinableLoops:

  • Not seen, but seems pretty arbitrary. Could be buggy. Worth a try, I suppose.

• Style/StringConcatenation:

  • Lots of these, potentially less readable. Unsafe auto-correctable, so worth a try.

• Lint/UselessMethodDefinition:

  • Seen in Whitehall, and potentially buggy / less readable. Suggest we turn it off.

• Lint/MissingSuper:

  • Lots of these, and potentially buggy - this is what tests are for. Suggest we turn it off.

• Style/SingleArgumentDig:

  • Seen in one of the email apps, potentially less readable. Unsafe auto-correctable, so worth a try.

• Style/HashLikeCase:

  • Not seen, seems very arbitrary. Suggest we turn it off if it causes a problem.

• RSpec/StubbedMock:

  • Agree as Kev described.

• Gemspec/RequiredRubyVersion:

  • Agree as Kev described.

• Naming/VariableNumber:

  • Lots of these, and agree it has already reduced readability. Since it's not autocorrectable, it would be expensive to switch in any direction, apart from disabling the new options. I think the styleguide is too prescriptive here, since it assumes all number in identifiers have no meaning, which is incorrect. Suggest we turn it off entirely.

• RSpec/MultipleMemoizedHelpers:

  • Not seen, agree as Kev described.

• Rails/LexicallyScopedActionFilter:

  • Not seen - pretty sure this is already enabled.

I think it's valid for this PR to go in as-is, since we seem to be near agreement with the follow-up changes to config, and provided @kevindew is happy to do look at those.

[theseanything]

@kevindew so I noticed you saw the mintiest suggestion in some repos. I don't think we shouldn't be enabling this option here, instead it should be added to the local project rubocop config as it feels like project specific annoyance. Possible that projects (in and outside GOV.UK) use this gem and extension suggestions would indeed be helpful.

This also has a larger impact of silencing not just suggestions for minitest, but all extension suggestions. I feel that at the very least, we could limit it to that:

AllCops:
  SuggestExtensions:
    rubocop-minitest: false

[kevindew]

Sure, I see the problem this option faces us with is one of consistency rather than it being a project annoyance, where the suggestion encourages you to take an approach inconsistent with the direction of this project. rubocop-govuk acts as a batteries included tool with various extensions and configs bundled that you can choose, and across GOV.UK there is a favouring (and lots of work) into trying to keep apps consistent with this styleguide rather than deviating in their own directions. This suggestion seems to encourage an app to go in its own direction and to end up with some extensions provided by this gem and others at an app level which seems potentially harmful.

I wouldn't have qualms with extra extensions, that are useful, being a part of this gem - minitest seems reasonable given a few of our apps use it. But I think having rubocop-govuk suggest ones to install at a project level is a step away from consistency.

[benthorner]

Yup, I also agree we should turn off suggestions: in addition to what @kevindew says, we're not acting on the ones we have, so it's not clear what the benefit of the suggestion is. We can always turn it on in future if we feel we've missed out on something in the majority of our repos.

[theseanything]

Thanks @csutter for raising this!

It's good to be moving to a stable releases of most of the dependencies. On the whole I think we should only disable cops for rules and styles we'd explicitly not like to add to our style guide, rather than rules that cause upgrading pain for our existing repositories. For example, the Lint/MissingSuper cop is useful to help enforce expected inheritance patterns - it may require substantial effort for some projects to fix, however the cop is doing what is intended (point out an style issue) and worse case that we disable the cop in specific projects if needed.

From reading the comments above, plus my own thoughts, we should only disable the following cops:

• Naming/VariableNumber - not adhered to by most GOV.UK projects and potentially not a style we'd like to introduce due to readability concerns.
• RSpec/MultipleMemoizedHelpers - there is precedence for not adding this type of rule to our style guide (as @kevindew said, we tend to not set arbitrary limits)

There was suggestion for disabling other following cops, but recommend we don't:

• RSpec/StubbedMock - we have widespread adherence across GOV.UK projects (only have a handful of offences in 2 projects: content-publisher (15), asset-manager (2)). Seems like a sensible rule (makes it unclear if the stub is part of the expectation).
• Lint/UselessMethodDefinition - we have widespread adherence across GOV.UK projects (21 offences, with 14 of them in Whitehall alone). It's auto-correctable.
• Lint/MissingSuper - we have broad adherence, and only 75 offences in GOV.UK projects (with majority in 2 projects, content-publisher and email-alert-api) - can switch it off in those project if need be. Seems sensible to help enforce expected inheritance patterns.

Also I think it would be good to break these changes into scoped commits. It'll provide a clear history and let us describe the reasoning for each change in the commit message. We could roughly scope them to following individual changes:

• Updating dependencies requirements (updating minimum versions and loosing rules)
• Disabling the SuggestExtensions (if we still want this)
• Setting Ruby version requirements
• Fixing namespace change of Capybara/FeatureMethods
• Disabling x cop (if we do)

[kevindew]

Thanks @theseanything - I think the most reasonable next step is to get this PR merged and then have a separate one for the rule changes.

It seems like everyone is pretty happy with disabling Naming/VariableNumber and RSpec/MultipleMemoizedHelper which seems pretty straight forward. I'll plan to open a PR for those two.

Sean's stats on Lint/UselessMethodDefinition and Lint/MissingSuper seem sufficient to justify that it's not as much of a pain to fix these as it might have seemed, can always disable later if they are buggy as feared.

That leaves one, RSpec/StubbedMock, left. I think I might have misinterpreted this rule in my first assessment as I thought it disallowed expect(x).to receive(y) but it seems to be a problem with and_return e.g. expect(x).to receive(y).and_return(z) which does seem more reasonable.

[kevindew]

I've opened #125 to disable the problematic rules.

[benthorner]

Thanks for the follow-up PR @kevindew! I'm going to add a couple of comments to that, as I still have concerns about "Lint/UselessMethodDefinition" and "Lint/MissingSuper", and more generally how we're going to apply this update so repos don't get left behind.

[willfish]

Any chance of releasing this, guys?

[willfish]

Also, thanks for massively updating this project with newer rubocop config

[kevindew]

Any chance of releasing this, guys?

We've released it today. It's currently 4.0.0.pre.1 while we trial the rules in our project. Hopefully within a couple of weeks we should release the final release. Do feel free to use the pre-release and to feedback if you find any of the new cops painful.