Skip to content
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

Adds the collection label to a document's data #2436

Merged
merged 10 commits into from
Jun 4, 2014
Merged

Adds the collection label to a document's data #2436

merged 10 commits into from
Jun 4, 2014

Conversation

budparr
Copy link

@budparr budparr commented May 23, 2014

Related to issue #52 in Jekyll/Jekyll-help this PR adds a document's collection label to its available data.

Forgive the noob, please: I tried adding a test test_document.rb in 'should know its data' and it failed, but I think because of the test, not the change. I ran the addition on a local install and it worked there.

@ct0512
Copy link

ct0512 commented May 23, 2014

What can you tell me where you people? Do you think this is much more difficult to repair? I translated it in English to express clear!

@parkr
Copy link
Member

parkr commented May 23, 2014

@ct0512 Can you please open a new issue describing your problem? Here: https://github.com/jekyll/jekyll/issues/new

@ct0512
Copy link

ct0512 commented May 23, 2014

You are?

 Want to know what?

------------------ Original ------------------
From: "notifications";notifications@github.com;
Date: Fri, May 23, 2014 01:24 PM
To: "jekyll/jekyll"jekyll@noreply.github.com;
Cc: "°®ÎÒÄã»áÍ´"laochenmxj@foxmail.com;
Subject: Re: [jekyll] Adds the collection label to a document's data (#2436)

@ct0512 Can you please open a new issue describing your problem? Here: https://github.com/jekyll/jekyll/issues/new

¡ª
Reply to this email directly or view it on GitHub.

@parkr
Copy link
Member

parkr commented May 23, 2014

@ct0512 I don't understand your problem. Can you please open a new issue: https://github.com/jekyll/jekyll/issues/new

@budparr budparr mentioned this pull request May 30, 2014
7 tasks
@@ -206,7 +206,8 @@ def to_liquid
"content" => content,
"path" => path,
"relative_path" => relative_path,
"url" => url
"url" => url,
"label" => collection.label
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just give the person the whole collection? (via collection.to_liquid)

@budparr
Copy link
Author

budparr commented May 30, 2014

Sure, but I thought you could derive everything else once you had the label.

@parkr
Copy link
Member

parkr commented May 30, 2014

I think I'd rather have document.collection.label, just so we know it's the collection label. document.label is a bit confusing to me.

@parkr
Copy link
Member

parkr commented May 30, 2014

Either that or document.collection is the collection label.

@budparr
Copy link
Author

budparr commented May 30, 2014

Yeah, I like this too. I'll make the change

On Friday, May 30, 2014, Parker Moore notifications@github.com wrote:

Either that or document.collection is the collection label.

Reply to this email directly or view it on GitHub
#2436 (comment).

Sent from Gmail Mobile

@parkr
Copy link
Member

parkr commented Jun 1, 2014

@budparr Once you can get this change in (and the associated test), I'll happily merge.

Bud Parr added 4 commits June 1, 2014 21:45
…into documenttoliquid

Conflicts:
lib/jekyll/document.rb
To make it clear the label is of the document's collection
@budparr
Copy link
Author

budparr commented Jun 2, 2014

Hi @parkr - I changed the key to collection from label. I still don't understand how to add the test because I would expect to be able to add it to the existing test for other items now in the to_liquid definition but don't understand how they are done. Apologies for the idiocy!

@@ -206,7 +206,8 @@ def to_liquid
"content" => content,
"path" => path,
"relative_path" => relative_path,
"url" => url
"url" => url,
"collection" => collection.label
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra space here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed, thank you

@parkr
Copy link
Member

parkr commented Jun 2, 2014

In terms of adding a test, write another should block in https://github.com/jekyll/jekyll/blob/7a9bc641ee30b5e926f22e39ddd35f94a96562dc/test/test_document.rb:

should "output the collection name in the #to_liquid method" do
  assert_equal @document.to_liquid['collection'], "methods"
end

Make sure it's inside that first context block and you should be good to go!

@budparr
Copy link
Author

budparr commented Jun 2, 2014

Thanks, that worked. I was leaving out "['collection']." Really appreciate it - I'm learning!

@parkr
Copy link
Member

parkr commented Jun 2, 2014

Thanks, that worked. I was leaving out "['collection']." Really appreciate it - I'm learning!

Yay! Glad it worked out properly. Happy to help 😄

parkr added a commit that referenced this pull request Jun 4, 2014
@parkr parkr merged commit 4e26874 into jekyll:master Jun 4, 2014
@budparr budparr deleted the documenttoliquid branch June 4, 2014 18:19
parkr added a commit that referenced this pull request Jun 4, 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants