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

Resolve #290 to inherit rubocop config from a gem. #2270

Merged
merged 1 commit into from
Oct 8, 2015

Conversation

jhansche
Copy link

The config directive should be in Hash format, with the gem_name
as the key, and the relative_path_to_config as the value. Ex:

inherit_from: .rubocop_todo.yml

inherit_gem:
    cucumber: config/rubocop.yml
    mygem: path/to/my/shared/rubocop.yml

If the gem is not installed, a warning is emitted, but execution
continues without the gem's config.

@jhansche
Copy link
Author

Working on the Ruby 1.9.3 failures. And I'll add a test for it as well

@bbatsov
Copy link
Collaborator

bbatsov commented Oct 6, 2015

Apart from the commit message and the missing changelog the PR looks reasonable to me. @jonas054 what do you think?

def resolve_inheritance_from_gems(hash, gems)
(gems || {}).each_pair do |gem_name, config_path|
hash['inherit_from'] ||= []
hash['inherit_from'] << gem_config_path(gem_name, config_path)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If hash['inherit_from'] already contains a string, e.g. .rubocop_todo.yml, this will concatenate to that string.

@jonas054
Copy link
Collaborator

jonas054 commented Oct 6, 2015

Super cool feature! I found one bug.

@jhansche
Copy link
Author

jhansche commented Oct 6, 2015

@bbatsov

Apart from the commit message

What specifically about the commit message is problematic? Would you prefer that I rewrite the commit message and/or amend the commit with all changes squashed (instead of keeping the chain of 3 commits once I fix Jonas's bug)?

@jonas054 Nice find.. Just tested and confirmed that it fails with the String (instead of Array) yaml syntax. I'll definitely clean it up to make it more explicit.

@jhansche jhansche force-pushed the inherit-gem branch 5 times, most recently from 787249e to 4ab8ae4 Compare October 6, 2015 18:51
@jhansche
Copy link
Author

jhansche commented Oct 6, 2015

@bbatsov

Offenses:
lib/rubocop/config_loader.rb:12:3: C: Class has too many lines. [148/147]
  class ConfigLoader
  ^^^^^
626 files inspected, 1 offense detected

Any suggestions for dropping 1 line from this class? 🎱

@bbatsov
Copy link
Collaborator

bbatsov commented Oct 7, 2015

Any suggestions for dropping 1 line from this class?

You can just increase to max class length.

Regarding the commit message - the title should not end with a .. Apart from that it looks OK.

@bbatsov
Copy link
Collaborator

bbatsov commented Oct 7, 2015

I also think it'd be a good idea to mention this feature somewhere in the README, otherwise most people will likely miss it.

@jhansche jhansche force-pushed the inherit-gem branch 2 times, most recently from 3aaa4e8 to 5a26e31 Compare October 7, 2015 16:37
@jhansche
Copy link
Author

jhansche commented Oct 7, 2015

@bbatsov Thanks! I've updated the max class length, fixed the trailing commit summary punctuation, and added a blurb to the README.

@jonas054
Copy link
Collaborator

jonas054 commented Oct 7, 2015

I don't see why we should warn and continue when a config file from a gem can't be found. We should fail just like when a file listed in inherit_from can't be found.

@jhansche
Copy link
Author

jhansche commented Oct 7, 2015

I don't see why we should warn and continue when a config file from a gem can't be found.

The main reason I went this route is for cases when you might need to run rubocop without first installing all dependencies. Granted, specifying inherit_gem may imply that running rubocop is dependent upon having that gem installed, so maybe in that case failing is the right approach... ¯_(ツ)_/¯

I don't mind either way. I'll make it fail instead of warning, if that is the consensus?

@bbatsov
Copy link
Collaborator

bbatsov commented Oct 7, 2015

I don't mind either way. I'll make it fail instead of warning, if that is the consensus?

Yeah, that would probably be best.

@jhansche
Copy link
Author

jhansche commented Oct 7, 2015

Sounds good -- done!

@jhansche
Copy link
Author

jhansche commented Oct 7, 2015

Fixing the rspec test...

The config directive should be in Hash format, with the `gem_name`
as the key, and the `relative_path_to_config` as the value. Ex:

    inherit_from: .rubocop_todo.yml

    inherit_gem:
        cucumber: config/rubocop.yml
        mygem: path/to/my/shared/rubocop.yml
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