Allow quoted date in front matter defaults #4184

Merged
merged 3 commits into from Dec 11, 2015

Conversation

Projects
None yet
4 participants
@ducktyper
Contributor

ducktyper commented Nov 25, 2015

Fix to #4068

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Nov 25, 2015

I think ensure_time! is a nice name, technically as it stands now this will turn DateTime and Date into Time, the bang (exclamation) at the end let us anybody looking at our API instantly know this method is destructive instantly, we need to emphasize code that speaks for itself.

I think ensure_time! is a nice name, technically as it stands now this will turn DateTime and Date into Time, the bang (exclamation) at the end let us anybody looking at our API instantly know this method is destructive instantly, we need to emphasize code that speaks for itself.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Nov 25, 2015

Contributor

❤️ for doing this @ducktyper

Contributor

envygeeks commented Nov 25, 2015

❤️ for doing this @ducktyper

@ducktyper

This comment has been minimized.

Show comment
Hide comment
@ducktyper

ducktyper Nov 25, 2015

Contributor

Yes! that is the better name. Thanks for the quick feedback.

Contributor

ducktyper commented Nov 25, 2015

Yes! that is the better name. Thanks for the quick feedback.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 25, 2015

Member

This seems like a lack of flexibility on the users' parts. What if a user wants a string there and now we force a date? YAML parses dates just fine: just don't quote it.

Member

parkr commented Nov 25, 2015

This seems like a lack of flexibility on the users' parts. What if a user wants a string there and now we force a date? YAML parses dates just fine: just don't quote it.

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Nov 28, 2015

Contributor

Personally if this were my site I would expect anything labled "date" to be turned into an Object and enforced as so, and if I want a string I would not use an internal front-matter variable that is expected to be a date.

Contributor

envygeeks commented Nov 28, 2015

Personally if this were my site I would expect anything labled "date" to be turned into an Object and enforced as so, and if I want a string I would not use an internal front-matter variable that is expected to be a date.

@ducktyper

This comment has been minimized.

Show comment
Hide comment
@ducktyper

ducktyper Dec 1, 2015

Contributor

If I understand the code correctly, in my opinion, I believe the value of "date" from default front-matter and front-matter (of each post or collection file) need to be handled in the same way, because they both can be the value of data["date"] in a Document object which is used as a Time object.

Since Jekyll converts the value of "date" in front-matter to a Time object already in the same way, it is logical to do the same to the default front-matter unless that is not the desirable behaviour.

Contributor

ducktyper commented Dec 1, 2015

If I understand the code correctly, in my opinion, I believe the value of "date" from default front-matter and front-matter (of each post or collection file) need to be handled in the same way, because they both can be the value of data["date"] in a Document object which is used as a Time object.

Since Jekyll converts the value of "date" in front-matter to a Time object already in the same way, it is logical to do the same to the default front-matter unless that is not the desirable behaviour.

lib/jekyll/frontmatter_defaults.rb
+ def ensure_time!(set)
+ return set unless set.key?('values') && set['values'].key?('date')
+ return set if set['values']['date'].is_a?(Time)
+ set['values']['date'] = Utils.parse_date(set['values']['date'], "Front matter defaults does not have a valid date.")

This comment has been minimized.

@parkr

parkr Dec 4, 2015

Member

Can you indicate something more helpful in this error message? If you have a 200-line config.yml, this is going to be an annoying error.

@parkr

parkr Dec 4, 2015

Member

Can you indicate something more helpful in this error message? If you have a 200-line config.yml, this is going to be an annoying error.

@ducktyper

This comment has been minimized.

Show comment
Hide comment
@ducktyper

ducktyper Dec 5, 2015

Contributor

Now the error message shows which front-matter default set to look for by showing the set.
Also found a grammar mistake. Sorry for my unprofessional writing skill.
The invalid date is shown from Utils.parse_date method so I did not need to re-display it.

Here is an example error message that could be generated.

         ERROR: YOUR SITE COULD NOT BE BUILT:
                ------------------------------------
                Invalid date '2000-01-0': An invalid date format was found in a front-matter default set: {"scope"=>{"path"=>""}, "values"=>{"layout"=>"post", "date"=>"2000-01-0"}}

Let me know if I missed something else or need more explanation.
Thanks for your feedback.

Contributor

ducktyper commented Dec 5, 2015

Now the error message shows which front-matter default set to look for by showing the set.
Also found a grammar mistake. Sorry for my unprofessional writing skill.
The invalid date is shown from Utils.parse_date method so I did not need to re-display it.

Here is an example error message that could be generated.

         ERROR: YOUR SITE COULD NOT BE BUILT:
                ------------------------------------
                Invalid date '2000-01-0': An invalid date format was found in a front-matter default set: {"scope"=>{"path"=>""}, "values"=>{"layout"=>"post", "date"=>"2000-01-0"}}

Let me know if I missed something else or need more explanation.
Thanks for your feedback.

parkr added a commit that referenced this pull request Dec 11, 2015

@parkr parkr merged commit 4f21458 into jekyll:master Dec 11, 2015

1 check passed

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

parkr added a commit that referenced this pull request Dec 11, 2015

@envygeeks

This comment has been minimized.

Show comment
Hide comment
@envygeeks

envygeeks Dec 28, 2015

Contributor

I just noticed this when the suite started failing, this is a bad test.

I just noticed this when the suite started failing, this is a bad test.

@ducktyper

This comment has been minimized.

Show comment
Hide comment
@ducktyper

ducktyper Dec 28, 2015

Contributor

I want to fix the bad test but could you enlighten me with little more detail?
I reviewed the tests and came with some ideas but I'm guessing I missed something else.

  1. Using .any? is more readable than .find with "assert" method because .any? returns true/false while .find returns page/nil
  2. Tests "not raise error" and "parse date" can be merged to reduce testing time since @site.process method takes some time to run.
Contributor

ducktyper commented Dec 28, 2015

I want to fix the bad test but could you enlighten me with little more detail?
I reviewed the tests and came with some ideas but I'm guessing I missed something else.

  1. Using .any? is more readable than .find with "assert" method because .any? returns true/false while .find returns page/nil
  2. Tests "not raise error" and "parse date" can be merged to reduce testing time since @site.process method takes some time to run.

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