Merge config recursively

[gpakosz]

I faced the following situation:

• I want nanoc.yaml to only define the rsync deployment url
• I want a parent nanoc.yaml to define other rsync options

---

parent.yaml

deploy:
  default:
    kind: rsync
    options:
      - --verbose
      - --progress
      - --recursive
      - --links
      - --chmod=D755,F644
      - --times
      - --partial
      - --progress
      - --compress
      - --delete-during
      - --force
      - --exclude='.git'

nanoc.yaml

deploy:
  default:
    dst: 'example.com:/srv/www'

parent_config_file: _factory/nanoc.yaml

Without merging hashes recursively, parent config gets completely overridden.

[gpakosz]

Please discuss the naming of merge_recursively as well as where it should be defined. At the time of writing this comment, I realize merge_recursively could be added to lib/nanoc/base/core_ext/hash.rb.

Also, I didn't find tests related to parent_config_file in configuration_spec.rb and I didn't know how do write a test that involves an existing parent config file.

[denisdefreyne]

I’m concerned about this change because it changes existing behavior (“overwrite” becomes “merge”), and the change is therefore not backwards-compatible.

An alternative suggestion would be to qualify how the parent file gets loaded. For instance:

parent_config_file:
  filename: _factory/nanoc.yaml
  method: merge

parent_config_file:
  filename: _factory/nanoc.yaml
  method: overwrite

[gpakosz]

Although this is a behavior change, I doubt anyone would really expect overwrite instead of merge. I also doubt this feature is widely used :)

See it as bug fix!

[denisdefreyne]

It turns out that parent_config_file is not documented at all.

[gpakosz]

Writing down here the fact that I scanned ~500 Nanoc sites on GitHub and none uses parent_config_file.

The idea behind parent_config_file is to have inheritable defaults that can be tweaked. I believe the default case is to union hashes but override values which is what #merge_recursively() achieves.

    def merge_recursively(c1, c2)
      c1.merge(c2) { |_, i1, i2| i1.is_a?(Hash) && i2.is_a?(Hash) ? merge_recursively(i1, i2) : i2 }
    end

If you really want to cancel out a hash value that has been populated by a parent config file, you can still do key: nil explicitly:

# this site doesn't want deployment at all
deploy: nil

parent_config_file: _common/nanoc.yaml

In that respect, this PR should be seen as bug fixing rather than enhancement. I don't see any advantage in complexifying parent_config_file's syntax.

[denisdefreyne]

Makes sense! Given that

• this hasn’t been documented
• making this change breaks no tests
• there is no usage of parent_config_file anywhere

… I’m OK with treating this as a bug fix instead.

The config loader specs are in config_loader_spec.rb; could you add relevant tests there?

[gpakosz]

I added a test for #merge() in configuration_spec.rb. I decided to test in configuration_spec.rb instead of config_loader_spec.rb because the guts of the change are really in Configuration#merge() behavior.

[denisdefreyne]

Looks good!

[gpakosz]

Thanks