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 #876] Deep merge hashes in configuration when overriding #910

Merged
merged 1 commit into from
Mar 21, 2014

Conversation

jonas054
Copy link
Collaborator

Deep (recursive) merge that was removed in #347 is added back. The data structure in the CollectionMethods config parameters is changed so PreferredMethods holds an array of single key/value hashes instead of a big hash. That way it doesn't get merged. PercentLiteralDelimiters will be merged.

@bbatsov
Copy link
Collaborator

bbatsov commented Mar 20, 2014

I've been thinking about this a bit more now and I think we can actually leave this as a hash and have the deep merging as well. We just need to do some extra params checks that would detect config like:

map: collect
inject: reduce
collect: map

and remove the the map: collect key-value pair:

inject: reduce
collect: map

This way users will be able to override just a portion of the defaults, which is a good thing IMO.

@jonas054
Copy link
Collaborator Author

That sounds like a pretty good idea. I'll make the change.

Deep (recursive) merge that was removed in rubocop#347 is added back. Some special handling is added in the CollectionMethods class so that
local settings really override the default ones even though its
config parameters are deep merged. The other cop with hash config
parameters, PercentLiteralDelimiters, will also get its config deep
merged.
@jonas054
Copy link
Collaborator Author

Updated according to @bbatsov's suggestion. This way default.yml stays unchanged.

bbatsov added a commit that referenced this pull request Mar 21, 2014
[Fix #876] Deep merge hashes in configuration when overriding
@bbatsov bbatsov merged commit e21354a into rubocop:master Mar 21, 2014
@bbatsov
Copy link
Collaborator

bbatsov commented Mar 21, 2014

👍

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.

2 participants