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

Move instances of `Time.parse` into a Utils method #2682

Merged
merged 3 commits into from Aug 6, 2014

Conversation

Projects
None yet
3 participants
@alfredxing
Member

alfredxing commented Aug 2, 2014

Use a centralized Utils.parse_date method to do all of the date parsing (except in one case where a special rescue is used). Also, add error messages where they didn't exist already.

Show outdated Hide outdated lib/jekyll/utils.rb
def parse_date(input, msg = "Invalid date: could not be parsed.")
Time.parse(input)
rescue ArgumentError
raise Errors::FatalException.new(msg)

This comment has been minimized.

@parkr

parkr Aug 2, 2014

Member

It would be awesome to always have the input string printed, so the user can search (ack/ag) for that string to quickly find & change it. What do you think?

@parkr

parkr Aug 2, 2014

Member

It would be awesome to always have the input string printed, so the user can search (ack/ag) for that string to quickly find & change it. What do you think?

This comment has been minimized.

@alfredxing

alfredxing Aug 2, 2014

Member

That would indeed be nice. I'll add it in.

@alfredxing

alfredxing Aug 2, 2014

Member

That would indeed be nice. I'll add it in.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 2, 2014

Member

This is looking awesome.

Member

parkr commented Aug 2, 2014

This is looking awesome.

Show input in error message
Show date input in error message to make it easier for user
to find infringing date/file
def parse_date(input, msg = "Input could not be parsed.")
Time.parse(input)
rescue ArgumentError
raise Errors::FatalException.new("Invalid date '#{input}': " + msg)

This comment has been minimized.

@parkr

parkr Aug 2, 2014

Member

What do you think about "#{msg} ('#{input}')"?

@parkr

parkr Aug 2, 2014

Member

What do you think about "#{msg} ('#{input}')"?

This comment has been minimized.

@parkr

parkr Aug 2, 2014

Member

Just thinking about it.

@parkr

parkr Aug 2, 2014

Member

Just thinking about it.

This comment has been minimized.

@alfredxing

alfredxing Aug 2, 2014

Member

I wanted it to sound natural so it could be understood easily, so I'm not sure about the parentheses around the input as this wouldn't really indicate what it is.

@alfredxing

alfredxing Aug 2, 2014

Member

I wanted it to sound natural so it could be understood easily, so I'm not sure about the parentheses around the input as this wouldn't really indicate what it is.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 2, 2014

Member

Ok, looks great to me. Can you please add tests for this method? :)

Member

parkr commented Aug 2, 2014

Ok, looks great to me. Can you please add tests for this method? :)

@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing Aug 2, 2014

Member

Test added. Do you think those are enough, or should I add more in test_posts, etc.?

Member

alfredxing commented Aug 2, 2014

Test added. Do you think those are enough, or should I add more in test_posts, etc.?

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Aug 3, 2014

Member

Nope, looks great. 👍 :shipit:

Member

parkr commented Aug 3, 2014

Nope, looks great. 👍 :shipit:

@alfredxing

This comment has been minimized.

Show comment
Hide comment
@alfredxing

alfredxing Aug 3, 2014

Member

Should be good to merge at your will. 🚢

Member

alfredxing commented Aug 3, 2014

Should be good to merge at your will. 🚢

@parkr parkr merged commit cb671ac into jekyll:master Aug 6, 2014

1 check passed

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

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