Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix clobbering inherit_from additions when not using Namespaces in the configs #3421

Merged

Conversation

NickLaMuro
Copy link
Contributor

@NickLaMuro NickLaMuro commented Aug 17, 2016

Overview

The current implementation of load_file will load the hash from a config, then apply the base_configs to that hash, before converting it to a proper Config object, which means we are comparing a vanilla Hash with a Config, so one will have the converted namespace Cop names, and the other will not. This can clobber inherited configuration since merging will happen in resolve_inheritance, but will just keep the keys from both and then when add_missing_namespaces is called, one will be dropped.

Since Config inherits from Hash, there shouldn't be a problem with doing the resolve_* methods after the config = Config.new line since all the normal Hash methods are available (.delete specifically).

Example (before this change)

Given the following files:

.rubocop.yml

inherit_from: ["base.yml"]

LineLength:
  Exclude: ["Gemfile"]

base.yml

LineLength:
  Max: 120
  • .rubocop.yml is loaded through the load_file method

  • it, in turn, loads base.yml when called in base_configs from resovle_inheritance

  • When add_missing_namespaces is called on base.yml, it converts {"LineLength" => {"Max" => 120}} to have "Metrics/LineLength", and deletes old "LineLength" key

  • The unconverted hash of '.rubocop.ymlis then merged with the convertedbase.yml`, and so the following hash exists:

    {
      "LineLength"         => {"Exclude" => ["Gemfile"]},
      "Metrics/LineLength" => {"Max"     => 120}
    }
  • When add_missing_namespaces is called on the now merged .rubocop.yml config, it will delete the changes from "Metrics/LineLength" => {"Max" => 120} when converting "LineLength" to have a namespace, and the default LineLength max value will be used instead.


Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Used the same coding conventions as the rest of the project.
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • All tests are passing.
  • The new code doesn't generate RuboCop offenses.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.

@NickLaMuro NickLaMuro force-pushed the fix-inherited-non-namespace-clobbering branch from 3c4d6b5 to 2788bf0 Compare August 17, 2016 17:00
NickLaMuro added a commit to NickLaMuro/manageiq that referenced this pull request Aug 17, 2016
There is currently a bug with `rubocop` that clobbers additions from
inherited files if Namespaces aren't used in the original config:

rubocop/rubocop#3421

While waiting on the above fix to be merged and released, converting to
using namespaces in the inherited file will avoid this issue and
shouldn't be a problem when upgrading in the future, allowing us to
inherit properly and maintain ManageIQ specific additions to the default
found in ManageIQ/Guides
@bdunne
Copy link

bdunne commented Aug 17, 2016

👍

@NickLaMuro NickLaMuro force-pushed the fix-inherited-non-namespace-clobbering branch from 2788bf0 to 58d94b6 Compare August 17, 2016 18:37
@NickLaMuro
Copy link
Contributor Author

<3 that there was a test to catch me spelling my username wrong in the CHANGELOG.md :D

The current implementation of `load_file` will load the hash from a
config, then apply the base_configs to that hash, before converting it
to a proper `Config` object.

This can clobber inherited configuration since merging will happen in
resolve_inheritance, but will just keep the keys from both and then when
`add_missing_namespaces` is called, one will be dropped.

Since `Config` inherits from `Hash`, there shouldn't be a problem with
doing the `resolve_*` methods after the `config = Config.new` line since
all the normal Hash methods are available (.delete specifically).
@NickLaMuro NickLaMuro force-pushed the fix-inherited-non-namespace-clobbering branch from 58d94b6 to 6616733 Compare August 17, 2016 18:45
@bbatsov bbatsov merged commit d956705 into rubocop:master Aug 20, 2016
@bbatsov
Copy link
Collaborator

bbatsov commented Aug 20, 2016

Nice catch! Thanks for fixing this!

@NickLaMuro
Copy link
Contributor Author

No problem, thanks for merging!

Neodelf pushed a commit to Neodelf/rubocop that referenced this pull request Oct 15, 2016
The current implementation of `load_file` will load the hash from a
config, then apply the base_configs to that hash, before converting it
to a proper `Config` object.

This can clobber inherited configuration since merging will happen in
resolve_inheritance, but will just keep the keys from both and then when
`add_missing_namespaces` is called, one will be dropped.

Since `Config` inherits from `Hash`, there shouldn't be a problem with
doing the `resolve_*` methods after the `config = Config.new` line since
all the normal Hash methods are available (.delete specifically).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants