Use auto generated Rubocop config #2418
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The Style/CaseIndentation Rubocop rule requires that `when` statements are indented at the same level as their parent `case` statement. However, in this case we are assigning the result of the `case` statement to a local variable which pushes the `case` keyword to a deep level of "indentation". I think rigidly applying the rule in this case would lead to code that was hard to read, so I've elected to wrap this `case` statement in a `begin` ... `end` block. This means that the `case` statement can be at a sensible level of indentation. Previously everything inside the `case` statement was indented an odd number of characters which was odd. This commit also fixes that problem.
Previously we were using the `--diff --cached` options with `govuk-lint-ruby` to only run the Rubocop checks against code that had been changed in the given branch i.e. it's a way to avoid having to fix all the violations up-front. However, I'm not convinced this is a very sensible approach, because it means we fix the violations in rather a piecemeal way. Also I think we've run into a catch-22 situation a couple of times where we wanted to revert a merge commit to put the code back *exactly* as it was previously. The problem is that the revert commit can draw Rubocop's attention to "old" lines of code that may not pass the Rubocop checks - thus preventing us from doing the straightforward revert. In this commit I've generated a project-specific `.rubocop_todo.yml` file using `govuk-lint-ruby --auto-gen-config --exclude-limit 99999` which excludes existing violations on a per-cop basis [1]. The limit was set to 99999 to minimise the number of cops that are disabled entirely - this way most cops are only disabled for a specific set of files. The generated `.rubocop_todo.yml` file is included into the configuration via a local `.rubocop.yml` file using an `inherit_from` key. This means that the exclusions in `.rubocop_todo.yml` are used for all `govuk-lint-ruby` commands by default. Note that the `.rubocop.yml` file does not *override* the cop definitions in the `govuk-lint` gem - it is merely a way to add *extra* configuration. Using this approach should prevent us from introducing new violations into the project without needing to fix all the existing violations first. However, in the long run, the idea is for us to gradually remove these exclusions to bring the project fully in line with the default `govuk-lint-ruby` configuration. This should be pretty straightforward, since we can just fix violations of a single cop or even just violations of a single cop in a single file and then update the `.rubocop_todo.yml` file accordingly. Ultimately we will be able to remove the `.rubocop_todo.yml` & `.rubocop.yml` files and just rely on the default `govuk-lint` configuration. Note that as per the header comment in `.rubocop_todo.yml`, changes in the inspected code, or installation of new versions of RuboCop, may require `.rubocop_todo.yml` to be generated again. I've also updated the `jenkins.sh` CI script and the relevant documentation. [1]: https://github.com/bbatsov/rubocop/blob/master/README.md#automatically-generated-configuration
I enabled the rule and ran the following command:
$ govuk-lint-ruby --auto-correct
I enabled the rule and ran the following command:
$ govuk-lint-ruby --auto-correct
I enabled the rule and ran the following command:
$ govuk-lint-ruby --auto-correct
I enabled the rule and ran the following command:
$ govuk-lint-ruby --auto-correct
I enabled the rule and ran the following command:
$ govuk-lint-ruby --auto-correct
I enabled the rule and ran the following command:
$ govuk-lint-ruby --auto-correct
I enabled the rule and ran the following command:
$ govuk-lint-ruby --auto-correct
The fixes for the Rubocop violations have affected quite a few flows. However, I've run all the regression tests locally and they all pass, so I'm happy to update the checksums.
By default Rubocop looks for Ruby files in all project directories. There seem to be some Ruby files relating to `govuk-content-schemas` in the project `tmp` directory on the Jenkins CI server which contain cop violations. We don't care about files in this directory, so we can exclude them from the Rubocop checks.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Supersedes #2401.
Motivation
Previously we were using the
--diff --cachedoptions withgovuk-lint-rubyto only run the Rubocop checks against code that had been changed in the given branch i.e. it was a way to avoid having to fix all the violations up-front.However, I'm not convinced this is a very sensible approach, because it means we fix the violations in rather a piecemeal way. Also I think we've run into a catch-22 situation a couple of times where we wanted to revert a merge commit to put the code back exactly as it was previously. The problem is that the revert commit can draw Rubocop's attention to "old" lines of code that may not pass the Rubocop checks - thus preventing us from doing the straightforward revert.
Trello card: https://trello.com/c/k4VNXtA2
Description
This PR takes a different approach as follows:
govuk-lint-ruby --auto-gen-configto generate.rubocop_todo.ymlfile to disable cops for existing violations.rubocop_todo.ymlinto configuration viainherit_fromkey in.rubocop.yml.rubocop_todo.ymljenkins.shCI script to stop using--diff --cachedoptionsThe reason that I fixed violations of the 7 cops in this PR was because I wanted to end up with a
rubocop_todo.ymlfile with no cops completely disabled. I was concerned that this would make it all to easy to accidentally introduce further violations of the disabled cops.Even so, this isn't a perfect solution, because it is still possible to accidentally introduce violations of the other cops in files which are already excluded. However, I still think this is a better approach.
Hopefully we can fix the majority of the violations in the not too distant future - I notice that 35 of the 47 cops remaining in the todo list allow auto-correction.
Internal changes
This PR should change the options for the
govuk-lint-rubycommand executed on the CI server.External changes
None.