filters: raise error if no input given to date filters #5127

Merged
merged 1 commit into from Jul 25, 2016

Conversation

Projects
None yet
3 participants
@ayastreb
Contributor

ayastreb commented Jul 23, 2016

Fixes #5107

lib/jekyll/filters.rb
- Jekyll.logger.error "Invalid Date:", "'#{input}' is not a valid datetime."
- exit(1)
+ raise Errors::InvalidDateError,
+ "Invalid Date: '#{input}' is not a valid datetime."

This comment has been minimized.

@ayastreb

ayastreb Jul 23, 2016

Contributor

I'm not sure about error message here.
Theoretically it should only get here if input is not given, so maybe it should be something like:
No input given to filter #{filter_name} in #{document_path} ?
Can we determine somehow the path of the document where the error is?

@ayastreb

ayastreb Jul 23, 2016

Contributor

I'm not sure about error message here.
Theoretically it should only get here if input is not given, so maybe it should be something like:
No input given to filter #{filter_name} in #{document_path} ?
Can we determine somehow the path of the document where the error is?

This comment has been minimized.

@parkr

parkr Jul 23, 2016

Member

@ayastreb When an error is raised in Liquid, the renderer catches that and prints the document for which the error occurred so that's all take care of. What's important is to ensure the input is displayed properly. How about "Invalid Date: \"#{input.inspect}\" is not a valid time." ?

@parkr

parkr Jul 23, 2016

Member

@ayastreb When an error is raised in Liquid, the renderer catches that and prints the document for which the error occurred so that's all take care of. What's important is to ensure the input is displayed properly. How about "Invalid Date: \"#{input.inspect}\" is not a valid time." ?

This comment has been minimized.

@parkr

parkr Jul 23, 2016

Member

Adding the .inspect call means instead of Invalid Date: "" is not a valid datetime., you'd see Invalid Date: "nil" is not a valid datetime.

@parkr

parkr Jul 23, 2016

Member

Adding the .inspect call means instead of Invalid Date: "" is not a valid datetime., you'd see Invalid Date: "nil" is not a valid datetime.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 23, 2016

Member

LGTM!

Member

parkr commented Jul 23, 2016

LGTM!

@parkr parkr added the ux label Jul 23, 2016

@parkr parkr added this to the 3.2 milestone Jul 23, 2016

@ayastreb

This comment has been minimized.

Show comment
Hide comment
@ayastreb

ayastreb Jul 23, 2016

Contributor

@parkr, yes that sounds good! I've chnaged the error message to include .inspect.

Contributor

ayastreb commented Jul 23, 2016

@parkr, yes that sounds good! I've chnaged the error message to include .inspect.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 23, 2016

Member

LGTM.

Member

parkr commented Jul 23, 2016

LGTM.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Jul 25, 2016

Member

@jekyllbot: merge +bug

Member

parkr commented Jul 25, 2016

@jekyllbot: merge +bug

@jekyllbot jekyllbot merged commit 57944c3 into jekyll:master Jul 25, 2016

1 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
jekyll/lgtm Approved by @parkr. Requires 1 more LGTM.
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jekyllbot jekyllbot added bug fix labels Jul 25, 2016

jekyllbot added a commit that referenced this pull request Jul 25, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment