Update rubocop-govuk to 4.0.0.pre.1 #6036

kevindew · 2021-03-23T10:45:36Z

Trello: https://trello.com/c/pqXbKZiC/215-test-rubocop-govuk-400pre1-prior-to-400-release

This bumps to the 4.0.0 pre release of rubocop-govuk which is part of trialling the new rubocop-govuk release. Like many updates of linting in Whitehall, this was pretty tough going - with a need to disable Style/OptionalBooleanParameter for Sidekiq workers and Rails/LexicallyScopedActionFilter.

Of particular challenge was the Style/StringConcatenation rule, which produced some risky fixes when html_safe strings were considered. I'm expecting that this won't be as hard to deal with when writing code for the first time.

The gem pin to 4.0.0.pre.1 is intended to be removed once the final release of rubocop-govuk 4.0.0 is out.

This is to test the pre-release out of 4.0.0. Further details are in: alphagov/rubocop-govuk#129

rubocop -A

This fixes a few places where Rubocop produced some undesirable code, often where the code was already rather complex. I've tried to make this code easier to read with these changes.

Rubocop has a cop that checks for shuffle instances that could be replaced with sample. The cop crashes with this code as it doesn't anticipate that an endless range could be provided. The code we have here is pretty bizarre, it shuffles and an array and then returns all but the first element. This seems a totally strange use case and I felt it was better to change this code rather than resolve the bug in Rubocop.

I couldn't find any indication that `document_collection_edition_with_state` was used in the app so I removed it rather than fix the violations. The topic_fragment and organisation_fragment seemed tough to mentally parse in their past guise so I have broken them down into procedural steps. There didn't seem to be any value in fixing the data migrations from 2016 so I have ignored their issues.

This task is not part of any production checks and seems to have just modified since 2012 for linting violations. It seems simpler to just remove it. We can always bring it back from the git history if we discover a need.

This complains about the use of filter as a struct attribute as it is also the name of a Struct instance methods [1]. [1]: https://ruby-doc.org/core-2.7.2/Struct.html#method-i-filter

The coding patterns for applying permissions to controllers are somewhat whacky in this app, using inheritance frequently as the means. This means that it's rather common to find violations of the Rails\LexicallyScopedActionFilter cop. I've decided to turn this rule off for this whole repo.

These constants aren't constrained to the block scope as a developer may have expected. This changes these mostly to variables which is scoped.

This offence was caused because this repo had an empty file committed. After having a look I decided this file didn't need to exist and changed this generator to accommodate this.

This fixes a couple of scenarios where Rubocop string interpolation rules broke the delicate balance of html_safe strings. The html_safe flag on strings can be quite nuanced. Examples: [1] pry(main)> ("My HTML".html_safe + "another").html_safe? => true [2] pry(main)> ("another" + "My HTML".html_safe).html_safe? => false [3] pry(main)> "#{'My HTML'.html_safe} another".html_safe? => false This leaves it rather vulnerable for Rubocop changes where in plain Ruby the strings would be equivalent.

This object is used in Rails caches, when it actually initializes the superclass this ends up in a non-serializable state. Presumably this has ended up being something of a half a view class. The simplest thing to do for now is ignore this violation.

It wasn't applicable to apply this to Sidekiq workers as they don't support keyword arguments.

This method looks way out of date and I can't find any usages of it or the file whitehall_secrets.yml. This removal was prompted by the Rubocop autocorrects in 2bf44e2 that created a bad path with string interpolation.

ChrisBAshton

Looks good to me. A couple of snag comments but nothing blocking.
This looked quite hairy - well done!

ChrisBAshton · 2021-03-25T11:06:19Z

app/helpers/document_helper.rb

@@ -245,7 +247,8 @@ def from_metadata(document, links_only = false)
      from += array_of_links_to_organisations(document.lead_organisations)
    end

-    if !document.respond_to?(:statistics?) && document.statistics? && document.respond_to?(:delivered_by_minister?)


Hmmm I don't understand this conditional - if a document doesn't respond to statistics, how can we then call document.statistics?? 🤔 Is this what Rubocop created?

(I agree your fix is an improvement in any case).

Yeah I think rubocop got it wrong - their fix should have been if !(document.respond_to?(:statistics?) && document.statistics?) && document.respond_to?(:delivered_by_minister?). Which would have been a pretty big mouthful.

ChrisBAshton · 2021-03-25T11:16:02Z

features/support/retrospective_stub.rb

-      call[:method] == method
+    if @calls.none? { |call| call[:method] == method }
+      raise UnsatisfiedAssertion,
+            "Expected :#{method} to have been called, but wasn't\n\nCalls: \n#{inspect_calls}"


Could this follow the same approach as the examples below? I'm finding it a bit tricky to parse.

methods_called = @calls.none? { |call| call[:method] == method } unless methods_called raise UnsatisfiedAssertion...# end

I think this is ok, it seems pretty idiomatic to have a one line block inside a conditional.

ChrisBAshton · 2021-03-25T13:22:30Z

test/unit/whitehall/form_builder_test.rb

+        '<input id="promotional_feature_item_remove_image" name="promotional_feature_item[remove_image]" type="checkbox" value="1" />' \
+        "#{label_text}" \
+      "</label>" \
+    "</div>"


ChrisBAshton · 2021-03-25T13:59:37Z

app/presenters/rummager_document_presenter.rb

@@ -10,6 +10,7 @@ class RummagerDocumentPresenter < ActionView::Base
  attr_reader :title, :link, :summary, :content_id

  def initialize(document_hash)
+    super()


Do we need these parentheses?

Yup, super is a special method, without parenthesis it will automatically pass in the methods arguments https://dannyh79.github.io/posts/ruby-super-super

ChrisBAshton · 2021-03-25T14:19:38Z

lib/generators/data_migration/data_migration_generator.rb

  def create_data_migration
-    migration_template "data_migration.rb", Rails.root.join("db/data_migration", "#{file_name}.rb")


Thanks for deleting this! 👏

ChrisBAshton · 2021-03-25T14:20:24Z

app/helpers/admin/audit_trail_helper.rb

@@ -6,7 +6,7 @@ def describe_audit_trail_entry(entry)
               entry.message
             end
           else
-             "#{tag.span(entry.action.capitalize, class: 'action')} by"
+             tag.span(entry.action.capitalize, class: "action") + " by" # rubocop:disable Style/StringConcatenation


Well explained. How weird!

ChrisBAshton · 2021-03-25T14:24:00Z

lib/whitehall.rb

@@ -132,10 +132,6 @@ def self.admin_root
    @admin_root ||= Plek.new.external_url_for("whitehall-admin")
  end

-  def self.secrets


Yes, looks like it's no longer used. It was added in 3be3fd6

kevindew · 2021-03-29T17:48:56Z

Thanks for the review @ChrisBAshton 👍

kevindew added 19 commits March 22, 2021 19:20

Install rubocop-govuk 4.0.0.pre.1

454765f

This is to test the pre-release out of 4.0.0. Further details are in: alphagov/rubocop-govuk#129

Auto-correct rubocop-govuk offences

2bf44e2

rubocop -A

Fix some of the less nice Rubocop autocorrections

0a58160

This fixes a few places where Rubocop produced some undesirable code, often where the code was already rather complex. I've tried to make this code easier to read with these changes.

Delete OrphanedAttachmentFinder

b5a28a3

This task is not part of any production checks and seems to have just modified since 2012 for linting violations. It seems simpler to just remove it. We can always bring it back from the git history if we discover a need.

Fix Lint/SelfAssignment Rubocop offences

d9490dd

Fix Style/HashLikeCase rubocop offences

fcecca1

Ignore Lint/StructNewOverride Rubocop offence

52069d1

This complains about the use of filter as a struct attribute as it is also the name of a Struct instance methods [1]. [1]: https://ruby-doc.org/core-2.7.2/Struct.html#method-i-filter

Resolve Lint/ConstantDefinitionInBlock Rubocop offences

e8fdfcf

These constants aren't constrained to the block scope as a developer may have expected. This changes these mostly to variables which is scoped.

Resolve Rails/FilePath Rubocop violations

f750582

Resolve Lint/MissingSuper Rubocop offences

562f671

Resolve Lint/EmptyFile Rubocop offence

aaf4013

This offence was caused because this repo had an empty file committed. After having a look I decided this file didn't need to exist and changed this generator to accommodate this.

Resolve duplicate require

5d0c9a6

Resolve Style/OptionalBooleanParameter Rubocop violations

cd718bf

It wasn't applicable to apply this to Sidekiq workers as they don't support keyword arguments.

Remove Whitehall.secrets

fb62cab

This method looks way out of date and I can't find any usages of it or the file whitehall_secrets.yml. This removal was prompted by the Rubocop autocorrects in 2bf44e2 that created a bad path with string interpolation.

ChrisBAshton approved these changes Mar 25, 2021

View reviewed changes

kevindew merged commit 4e7b3b2 into master Mar 29, 2021

kevindew deleted the rubocop-v4 branch March 29, 2021 17:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update rubocop-govuk to 4.0.0.pre.1 #6036

Update rubocop-govuk to 4.0.0.pre.1 #6036

kevindew commented Mar 23, 2021

ChrisBAshton left a comment

ChrisBAshton Mar 25, 2021

kevindew Mar 29, 2021

ChrisBAshton Mar 25, 2021

kevindew Mar 29, 2021

ChrisBAshton Mar 25, 2021

ChrisBAshton Mar 25, 2021

kevindew Mar 29, 2021

ChrisBAshton Mar 25, 2021

ChrisBAshton Mar 25, 2021

ChrisBAshton Mar 25, 2021

kevindew commented Mar 29, 2021

		def create_data_migration
		migration_template "data_migration.rb", Rails.root.join("db/data_migration", "#{file_name}.rb")

Update rubocop-govuk to 4.0.0.pre.1 #6036

Update rubocop-govuk to 4.0.0.pre.1 #6036

Conversation

kevindew commented Mar 23, 2021

ChrisBAshton left a 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

kevindew commented Mar 29, 2021