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

Merge config recursively #1176

Merged
merged 1 commit into from May 13, 2017

Conversation

Projects
None yet
2 participants
@gpakosz
Member

gpakosz commented May 7, 2017

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

This comment has been minimized.

Show comment
Hide comment
@gpakosz

gpakosz May 7, 2017

Member

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.

Member

gpakosz commented May 7, 2017

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.

@ddfreyne

This comment has been minimized.

Show comment
Hide comment
@ddfreyne

ddfreyne May 8, 2017

Member

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
Member

ddfreyne commented May 8, 2017

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

This comment has been minimized.

Show comment
Hide comment
@gpakosz

gpakosz May 8, 2017

Member

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!

Member

gpakosz commented May 8, 2017

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!

@ddfreyne

This comment has been minimized.

Show comment
Hide comment
@ddfreyne

ddfreyne May 8, 2017

Member

It turns out that parent_config_file is not documented at all.

Member

ddfreyne commented May 8, 2017

It turns out that parent_config_file is not documented at all.

@gpakosz

This comment has been minimized.

Show comment
Hide comment
@gpakosz

gpakosz May 9, 2017

Member

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.

Member

gpakosz commented May 9, 2017

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.

@ddfreyne

This comment has been minimized.

Show comment
Hide comment
@ddfreyne

ddfreyne May 10, 2017

Member

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?

Member

ddfreyne commented May 10, 2017

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

This comment has been minimized.

Show comment
Hide comment
@gpakosz

gpakosz May 11, 2017

Member

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.

Member

gpakosz commented May 11, 2017

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.

@ddfreyne

This comment has been minimized.

Show comment
Hide comment
@ddfreyne

ddfreyne May 13, 2017

Member

Looks good!

Member

ddfreyne commented May 13, 2017

Looks good!

@ddfreyne ddfreyne merged commit 182de20 into nanoc:master May 13, 2017

3 checks passed

codecov/patch 100% of diff hit (target 98.69%)
Details
codecov/project Absolute coverage decreased by -<.01% but relative coverage increased by +1.3% compared to 0dfda17
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gpakosz

This comment has been minimized.

Show comment
Hide comment
@gpakosz

gpakosz May 13, 2017

Member

Thanks

Member

gpakosz commented May 13, 2017

Thanks

@gpakosz gpakosz deleted the gpakosz:merge-config-recursively branch Nov 2, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment