Allow documents to override output property in front-matter #3172

Merged
merged 2 commits into from Jan 6, 2015

Conversation

Projects
None yet
5 participants
@alfredxing
Member

alfredxing commented Nov 29, 2014

Allow documents to override the collection's output property in its front-matter. Fixes #2956. Should be a non-breaking change.

test/test_document.rb
+ @site = Site.new(Jekyll.configuration({
+ "collections" => {
+ "slides" => {
+ "output" => true

This comment has been minimized.

@XhmikosR

XhmikosR Nov 29, 2014

Contributor

Wrong indentation :)

@XhmikosR

XhmikosR Nov 29, 2014

Contributor

Wrong indentation :)

This comment has been minimized.

@alfredxing

alfredxing Nov 29, 2014

Member

Thanks! 👀

@alfredxing

alfredxing Nov 29, 2014

Member

Thanks! 👀

@XhmikosR

This comment has been minimized.

Show comment
Hide comment
@XhmikosR

XhmikosR Nov 29, 2014

Contributor

Shouldn't the output internal variable be documented somewhere? I mean, is there a list of those?

Contributor

XhmikosR commented Nov 29, 2014

Shouldn't the output internal variable be documented somewhere? I mean, is there a list of those?

@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing Nov 29, 2014

Member

@XhmikosR You mean the one in _config.yml?

Member

alfredxing commented Nov 29, 2014

@XhmikosR You mean the one in _config.yml?

@XhmikosR

This comment has been minimized.

Show comment
Hide comment
@XhmikosR

XhmikosR Nov 29, 2014

Contributor

I mean, there are some front matter variables, like output now, name etc.

PS. squash is nice :P

Contributor

XhmikosR commented Nov 29, 2014

I mean, there are some front matter variables, like output now, name etc.

PS. squash is nice :P

@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing Nov 29, 2014

Member

Yeah, that's a problem now, since the document.output variable is supposed to produce the rendered output of the document... (as per http://jekyllrb.com/docs/collections/#documents).

I usually squash/rebase when it's pretty much approved/ready to ship.

Member

alfredxing commented Nov 29, 2014

Yeah, that's a problem now, since the document.output variable is supposed to produce the rendered output of the document... (as per http://jekyllrb.com/docs/collections/#documents).

I usually squash/rebase when it's pretty much approved/ready to ship.

@XhmikosR

This comment has been minimized.

Show comment
Hide comment
@XhmikosR

XhmikosR Nov 29, 2014

Contributor

OK, cool, I expected those variables to be listed somewhere else and not in collections.

Contributor

XhmikosR commented Nov 29, 2014

OK, cool, I expected those variables to be listed somewhere else and not in collections.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 30, 2014

Member

This is not the approach I was thinking, because this still leaves the document in the collection's list of docs. Check out the Publisher class and how it's used to filter out posts and apply that logic – I think that's what @gjtorikian was looking for.

Member

parkr commented Nov 30, 2014

This is not the approach I was thinking, because this still leaves the document in the collection's list of docs. Check out the Publisher class and how it's used to filter out posts and apply that logic – I think that's what @gjtorikian was looking for.

@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing Nov 30, 2014

Member

@parkr Right. I'll change it up!

Member

alfredxing commented Nov 30, 2014

@parkr Right. I'll change it up!

@parkr parkr added the fix label Dec 1, 2014

@parkr parkr added this to the 2.6.0 milestone Dec 1, 2014

lib/jekyll/collection.rb
@@ -40,7 +40,7 @@ def read
if Utils.has_yaml_header? full_path
doc = Jekyll::Document.new(full_path, { site: site, collection: self })
doc.read
- docs << doc
+ docs << doc if publisher.publish?(doc)

This comment has been minimized.

@parkr

parkr Dec 1, 2014

Member

So please expose site.publisher and move this to use that directly.

@parkr

parkr Dec 1, 2014

Member

So please expose site.publisher and move this to use that directly.

@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing Dec 1, 2014

Member

Done!

Member

alfredxing commented Dec 1, 2014

Done!

@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing Dec 1, 2014

Member

Rebased & squashed :shipit:

Member

alfredxing commented Dec 1, 2014

Rebased & squashed :shipit:

@parkr parkr modified the milestones: 2.6.0, 3.0 Dec 29, 2014

@mattr-

This comment has been minimized.

Show comment
Hide comment
@mattr-

mattr- Jan 2, 2015

Member

@alfredxing one more rebase so it'll merge?

Member

mattr- commented Jan 2, 2015

@alfredxing one more rebase so it'll merge?

@parkr

This comment has been minimized.

Show comment
Hide comment
Member

parkr commented Jan 2, 2015

:shipit:

@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing Jan 5, 2015

Member

Awesome, I'll rebase once I get home from my vacation this afternoon!

Member

alfredxing commented Jan 5, 2015

Awesome, I'll rebase once I get home from my vacation this afternoon!

@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing Jan 6, 2015

Member

Rebased & ready for 🚢

Member

alfredxing commented Jan 6, 2015

Rebased & ready for 🚢

@parkr parkr merged commit 502fd94 into jekyll:master Jan 6, 2015

1 check passed

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

parkr added a commit that referenced this pull request Jan 6, 2015

@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.