Front-matter defaults inside documents of a collection #2419

Merged
merged 6 commits into from Jun 13, 2014

Conversation

Projects
None yet
5 participants
@jens-na

jens-na commented May 17, 2014

This PR adds support for frontmatter-defaults inside collections.

Currently you can specify default values in pages, posts and drafts only. With this PR it is possible to pass a collection to the the defaults/scope/type key in _config.yml.

Example:

You have a collection with two documents:

_slides/
  slide-1.html
  slide-2.html

And a _config.yml with this content:

...
collections:
  slides:
    output: true

defaults:
  -
    scope:
      path: ""
      type: slides   # <= here you can specify a collection now!
    values:
      layout: slide
      test: Value

You can use {{ page.test }} inside _slides/slide-1.html which will result to Value.
I have also written a few tests, but I like to write a few more.

Open issues:

  • Implementation
  • Add a few more tests, especially to make sure the defaults/scope/path key works with this PR
  • Create documentation
  • Rebase

See also: #2405
/cc @parkr @benbalter

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr May 20, 2014

Member

This looks pretty good to me. Should fix #2322.

Member

parkr commented May 20, 2014

This looks pretty good to me. Should fix #2322.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr May 20, 2014

Member

/cc @benbalter: what do you think about this?

Member

parkr commented May 20, 2014

/cc @benbalter: what do you think about this?

- @data = SafeYAML.load($1)
+ data_file = SafeYAML.load($1)
+ unless data_file.nil?
+ @data = Utils.deep_merge_hashes(defaults, data_file)

This comment has been minimized.

@mattr-

mattr- May 21, 2014

Member

Do we have a test for this particular conditional case? It seems like that merging a hash with nil should just give us the hash back. If that's the case, then the conditional wrapping this line of code can go away. 😃

@mattr-

mattr- May 21, 2014

Member

Do we have a test for this particular conditional case? It seems like that merging a hash with nil should just give us the hash back. If that's the case, then the conditional wrapping this line of code can go away. 😃

This comment has been minimized.

@jens-na

jens-na May 21, 2014

That's what I also expected when I wrote this patch, but unfortunately we need the additional condition because Utils.deep_merge_hashes returns nil if I want to merge a hash with nil.

@jens-na

jens-na May 21, 2014

That's what I also expected when I wrote this patch, but unfortunately we need the additional condition because Utils.deep_merge_hashes returns nil if I want to merge a hash with nil.

test/test_document.rb
+ }, @document.data)
+ end
+
+ should "overwrite a default value in the document frontmatter" do

This comment has been minimized.

@mattr-

mattr- May 21, 2014

Member

Let's remove these tests if they don't apply, or implement them if they do apply. 😃

@mattr-

mattr- May 21, 2014

Member

Let's remove these tests if they don't apply, or implement them if they do apply. 😃

This comment has been minimized.

@jens-na

jens-na May 21, 2014

I need to implement these tests. 😄

@jens-na

jens-na May 21, 2014

I need to implement these tests. 😄

@jens-na jens-na referenced this pull request May 30, 2014

Closed

Release Jekyll 2.1.0 #2465

5 of 7 tasks complete
@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jun 1, 2014

Member

This needs a rebase!

Member

parkr commented Jun 1, 2014

This needs a rebase!

features/frontmatter_defaults.feature
+ And I have a "_slides/slide2.html" file with content:
+ """
+ ---
+ myval: Override

This comment has been minimized.

@parkr

parkr Jun 1, 2014

Member

Should this extra indentation be here?

@parkr

parkr Jun 1, 2014

Member

Should this extra indentation be here?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jun 1, 2014

Member

With the rebase, this LGTM. 👍 :shipit:

@mattr-, any other thoughts on this one?

Member

parkr commented Jun 1, 2014

With the rebase, this LGTM. 👍 :shipit:

@mattr-, any other thoughts on this one?

@jens-na

This comment has been minimized.

Show comment
Hide comment
@jens-na

jens-na Jun 1, 2014

Rebased!

jens-na commented Jun 1, 2014

Rebased!

@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- Jun 7, 2014

Member

👍 :shipit:

Member

mattr- commented Jun 7, 2014

👍 :shipit:

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jun 7, 2014

Member

@mattr- Thanks!!

@benbalter Can I get your 👀 on this plz? ❤️

Member

parkr commented Jun 7, 2014

@mattr- Thanks!!

@benbalter Can I get your 👀 on this plz? ❤️

@benbalter

This comment has been minimized.

Show comment
Hide comment
@benbalter

benbalter Jun 13, 2014

Contributor
type: slides   # <= here you can specify a collection now!

👍

Contributor

benbalter commented Jun 13, 2014

type: slides   # <= here you can specify a collection now!

👍

@parkr parkr merged commit d59b2c3 into jekyll:master Jun 13, 2014

1 check passed

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

parkr added a commit that referenced this pull request Jun 13, 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.