Address linting offenses #3666
Address linting offenses #3666
Conversation
andrewgarner
force-pushed
the
fix-govuk-lint-offenses
branch
from
adbbf55
to
29e274c
Compare
There was a problem hiding this comment.
This is amazing. Thanks for taking this on!
However, rubocop can sometimes do strange things when running the Layout and Style cops, particularly in respect to indentation. It seems that the order the cops are run can leave things in a strange state. For example the changes introduced by Fix Style/ConditionalAssignment offenses have left some strange indenting around if/else/end blocks. I think they'd be cleaned up by one of the Layout/Indent* cops, but they were run in a previous commit. We may need to run multiple passes of the linter over the code in order to get a clean report from it. We will also want to hand review the changes even when we have a clean build because we sometimes get strange results that pass (I've highlighted a few in separate comments).
| @@ -25,7 +25,7 @@ def is_home_page_list_controller_for(list_name, opts) | |||
| redirect_to redirect_proc.call(home_page_list_container, home_page_list_item), notice: %{#{plural_name.titleize} on home page reordered successfully} | |||
| end | |||
|
|
|||
| protected | |||
| protected | |||
app/models/home_page_list.rb
Outdated
| @@ -93,11 +93,11 @@ def has_home_page_list_of(list_type) | |||
| plural_name = list_type.to_s | |||
| list_name = list_type.to_s | |||
| home_page_list_methods = Module.new do | |||
| protected | |||
| protected | |||
There was a problem hiding this comment.
The indenting here seems unusual, perhaps the method definitions should also be indented as much as this protected statement, or the Module.new do should be on a new line?
lib/address_formatter/h_card.rb
Outdated
| @@ -21,7 +21,7 @@ def interpolate_address_property(property_name) | |||
|
|
|||
| def replace_newlines_with_break_tags(string) | |||
| string. | |||
| gsub(/^\n/, ''). # get rid of blank lines | |||
| gsub(/^\n/, ''). # get rid of blank lines | |||
There was a problem hiding this comment.
The intention here is that this comment should line up with the comments on the next two lines. It seems rubocop understood that about the next two, but somehow messed up this one. Probably best to re-indent this one so it lines up.
| @@ -7,8 +7,8 @@ | |||
|
|
|||
| unless html_attachment.govspeak_content.present? | |||
| govspeak_content = html_attachment.build_govspeak_content( | |||
| body: html_attachment.attributes['body'], | |||
There was a problem hiding this comment.
All of the changes in this commit would benefit from going over by hand to line things up more readibly. Rubocop seems to have given us:
something = some_method_call(
argument_1,
argument_2,
argument_3
)when something like:
something = some_method_call(
argument_1,
argument_2,
argument_3
)or:
something =
some_method_call(
argument_1,
argument_2,
argument_3
)would be more readable.
| @@ -126,7 +126,7 @@ def edition_type_options_for_select(user, selected) | |||
| [edition_type.model_name.human.pluralize, edition_type.model_name.singular] | |||
| end | |||
| end | |||
| ActiveModel::Name | |||
| ActiveModel::Name | |||
There was a problem hiding this comment.
This line seems unusual, we're just referencing an ActiveModel constant that should already be loaded as part of rails. I think it could be deleted without any problems (worth checking git blame to see if there are hints as to why it's here, but I suspect it's a copy-paste error.
lib/tasks/cucumber.rake
Outdated
|
|
||
| vendored_cucumber_bin = Dir["#{Rails.root}/vendor/{gems,plugins}/cucumber*/bin/cucumber"].first | ||
| vendored_cucumber_bin = Dir["#{Rails.root}/vendor/{gems,plugins}/cucumber*/bin/cucumber"].first |
There was a problem hiding this comment.
In general it feels dangerous to make any changes to this file, because it makes it harder to upgrade cucumber and easily see the diff between the new version from the gem and our version. I wonder if we could disable rubocop for this file? If we decide to lint it though, the indent added here should be applied to the whole file as it's all inside the unless statement on line 8.
test/unit/consultation_test.rb
Outdated
| create(:nation_inapplicability, nation_id: Nation.scotland.id, alternative_url: "http://scot.gov.uk")] | ||
| ) | ||
| create(:nation_inapplicability, nation_id: Nation.scotland.id, alternative_url: "http://scot.gov.uk") | ||
| ]) |
There was a problem hiding this comment.
It would be better to break up and indent this ]) so the ] lines up with the nation_inapplicabilities: [ that opened it and the ) lines up with the create( that opened it (as they're on the same line we might want to break them up too).
| consultation | ||
| publication | ||
| detailed_guide | ||
| speech].each do |document_with_policies| |
There was a problem hiding this comment.
this might be better expressed as:
%i[
news_article
#...
speech
].each do |document_with_policites|
test "....." do
# whatever
end
endI think the single char indents have confused rubocop
| test 'cannot modify historic editions' do | ||
| refute enforcer_for(managing_editor, historic_edition).can?(:modify) | ||
| test 'cannot modify historic editions' do | ||
| refute enforcer_for(managing_editor, historic_edition).can?(:modify) |
There was a problem hiding this comment.
We need to indent the contents of this test too.
tmp_spec.rb
Outdated
| @@ -0,0 +1,38 @@ | |||
| require("rails_helper") | |||
There was a problem hiding this comment.
This file is introduced as the only change in a commit called Fix Performance/HashEachMethods offenses. Either the commit should be re-written to explain what this test is, or we shoudl drop the commit, and this test.
|
@h-lame thanks for the comments! I'm currently going through each of these commits one by one to sanity check the Rubocop autofixes (the commits for which I initially automated). You're right in spotting that some of the fixes have introduced additional style violations as a side effect so I'm addressing those at the same time. |
andrewgarner
force-pushed
the
fix-govuk-lint-offenses
branch
25 times, most recently
from
6d6d79f
to
255535c
Compare
andrewgarner
force-pushed
the
fix-govuk-lint-offenses
branch
from
255535c
to
5528f5b
Compare
The excluded files in the Robocop configuration file contain a `method_missing` method without also defining `respond_to_missing?`. It would be preferable to introduce the `respond_to_missing?` in each of the excluded files, however due to time constraints I was unable to do so in this commit. The cop will apply if any further `method_missing` methods are introduced however additional work will be needed to address the excluded files.
andrewgarner
force-pushed
the
fix-govuk-lint-offenses
branch
from
d3c8183
to
85f3e73
Compare
This addresses all 8,109 offences detected by Rubocop.