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

Set categories on post if they appear in site frontmatter defaults #2373

Merged
merged 3 commits into from May 21, 2014

Conversation

Projects
None yet
3 participants
@tschmidt
Contributor

tschmidt commented May 9, 2014

This pull request is the result of a discussion on #2343.

If category or categories were set in the site frontmatter defaults, they were not being picked up by the post when the site was being built. This was preventing the proper subdirectories from being built.

@parkr

This comment has been minimized.

Member

parkr commented May 11, 2014

@tschmidt

This comment has been minimized.

Contributor

tschmidt commented May 16, 2014

Hey guys, just dropping in to check on this. Any thoughts/feedback?

case hash[plural_key]
array = []
array << value_from_singular_key(hash, singular_key)
array << value_from_plural_key(hash, plural_key) if array.flatten.compact.empty?

This comment has been minimized.

@parkr

parkr May 20, 2014

Member

Why not value_from_singular_key(hash, singular_key) || value_from_plural_key(hash, plural_key)?

@tschmidt

This comment has been minimized.

Contributor

tschmidt commented May 20, 2014

Unfortunately, that won't work because #value_from_singular_key() may return [] or [nil], which will be treated as true setting the array variable and skip the call to #value_from_plural_key().

The way that I interpreted the description for #pluralized_array_from_hash() is that if the singular key does not return a value (e.g. it returns [] or [nil]) then try the plural key.

I could update the code for #value_from_singular_key() and #value_from_plural_key() so that they return nil as a value - instead of an array - which would allow for the single line of code. I didn't want to do that though because I think the intent of the code is clearer with two lines.

I will defer to you guys as far as the coding style, though. Let me know what you prefer :)

@tschmidt

This comment has been minimized.

Contributor

tschmidt commented May 20, 2014

For clarity, I've added a diff of what changes could be made to support a one-liner. I've also cleaned up the #pluralized_array_from_hash just a little by using the Object#tap method.

diff --git a/lib/jekyll/utils.rb b/lib/jekyll/utils.rb
index 6dab612..e1b4704 100644
--- a/lib/jekyll/utils.rb
+++ b/lib/jekyll/utils.rb
@@ -35,14 +35,13 @@ module Jekyll
       #
       # Returns an array
       def pluralized_array_from_hash(hash, singular_key, plural_key)
-        array = []
-        array << value_from_singular_key(hash, singular_key)
-        array << value_from_plural_key(hash, plural_key) if array.flatten.compact.empty?
-        array.flatten.compact
+        [].tap do |array|
+          array << (value_from_singular_key(hash, singular_key) || value_from_plural_key(hash, plural_key))
+        end.flatten.compact
       end

       def value_from_singular_key(hash, key)
-        [hash[key]] if hash.has_key?(key) || (hash.default_proc && hash[key])
+        hash[key] if (hash.has_key?(key) || (hash.default_proc && hash[key]))
       end

       def value_from_plural_key(hash, key)
@parkr

This comment has been minimized.

Member

parkr commented May 20, 2014

Thanks for looking into it. Kind of wish value_from_singular_key would return nil if it's not there. In any even, I appreciate your [].tap solution. Just looking to be as functional as possible in Jekyll – it makes things a bit easier to read. 😄

@tschmidt

This comment has been minimized.

Contributor

tschmidt commented May 20, 2014

After thinking about this more, I agree that value_from_singular_key should just return the value, or nil if it's not there. I'm going to go ahead and add the changes from the above diff. 😃

@parkr

This comment has been minimized.

Member

parkr commented May 20, 2014

👍 😄

Update `Utils#pluralized_array_from_hash` and `Utils#value_from_singu…
…lar_key` per suggestion from @parkr

Switched to using the `#tap` method for more concise code. Also returning the value from
`value_from_singular_key` instead of returning an array wrapped presentation of the value.
This allows for a one-liner in `pluralized_array_from_hash`.
@parkr

This comment has been minimized.

Member

parkr commented May 20, 2014

Cool, this looks good to me. Think we should update the documentation? Anything you can see in the configuration.md file that looks wrong to you?

@tschmidt

This comment has been minimized.

Contributor

tschmidt commented May 20, 2014

I don't see anything that looks wrong. In the section about Front-matter defaults, it's not really mentioned that you can set the category or categories. Currently, only layout and author are being used as examples as to what you can set in the defaults array. We could add a comment specifically about specifying the categories as well, but I think that might be overkill since you should technically be able to set any key/value pair there.

@parkr

This comment has been minimized.

Member

parkr commented May 20, 2014

Maybe you can add a note about the cascade?

@tschmidt

This comment has been minimized.

Contributor

tschmidt commented May 21, 2014

Will do. I'll push that sometime tomorrow.

@parkr

This comment has been minimized.

Member

parkr commented May 21, 2014

Awesome, thanks! We're trying to make sure master's docs are consistent with master's code. 😄 Also, this plural vs. singular stuff is a bit confusing without diving into the code, so it's always good to have a little extra help when reading up on it.

parkr added a commit that referenced this pull request May 21, 2014

@parkr parkr merged commit b9c3d8b into jekyll:master May 21, 2014

1 check passed

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

parkr added a commit that referenced this pull request May 21, 2014

@parkr

This comment has been minimized.

Member

parkr commented May 21, 2014

If you wouldn't mind following up with some docs, that'd be ⭐️!

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