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

Deep merge front matter defaults #2490

Merged
merged 3 commits into from Jun 25, 2014

Conversation

Projects
None yet
3 participants
@doktorbro
Member

doktorbro commented Jun 8, 2014

Overriding configuration is not mighty enough. Let’s deep merge it.

@parkr

View changes

features/frontmatter_defaults.feature Outdated
@@ -77,3 +77,9 @@ Feature: frontmatter defaults
Then I should see "a blog by some guy" in "_site/frontmatter.html"
And I should see "nothing" in "_site/override.html"
But the "_site/perma.html" file should not exist
Scenario: Deep merge frontmatter defaults
Given I have an "index.html" page with {juice: {orange: 1}} that contains "Juice: {{ page.juice.orange | plus: page.juice.apple }}"

This comment has been minimized.

@parkr

parkr Jun 8, 2014

Member

Looks like there's something wrong with this line in the tests.

@parkr

View changes

lib/jekyll/frontmatter_defaults.rb Outdated
old_scope = set['scope']
else
defaults = set['values'].merge(defaults)
defaults = set['values'].deep_merge(defaults)

This comment has been minimized.

@parkr

parkr Jun 8, 2014

Member

We use Jekyll::Utils.deep_merge_hashes instead.

@doktorbro

This comment has been minimized.

Member

doktorbro commented Jun 9, 2014

@parkr The tests pass.

@parkr

This comment has been minimized.

Member

parkr commented Jun 9, 2014

Great, thanks!

@doktorbro

This comment has been minimized.

Member

doktorbro commented Jun 10, 2014

@parkr Is this enhancement something you are interested in?

I’m building the API for a Jekyll plugin. It affects HTML files only. On Jekyll’s 1.5.x branch I can provide global settings in the config file like this:

# _config.yml
settings:
  foo: bar

If the same setting exists in a page’s front matter, I take this one.

With the merging technique I would just ask the user to put the settings into the front matter defaults. This would be more flexible.

@parkr

This comment has been minimized.

Member

parkr commented Jun 23, 2014

@penibelst I'd love to merge this change. Would you please rebase on current master? Danke!

@doktorbro

This comment has been minimized.

Member

doktorbro commented Jun 24, 2014

@parkr Rebased.

@parkr parkr merged commit 2018092 into jekyll:master Jun 25, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

parkr added a commit that referenced this pull request Jun 25, 2014

@doktorbro doktorbro deleted the doktorbro:deep-defaults branch Jun 25, 2014

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

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