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

Better error message when time is not parseable #1847

Merged
merged 1 commit into from Dec 24, 2013

Conversation

Projects
None yet
4 participants
@schneems
Contributor

schneems commented Dec 18, 2013

Give the full path of the file that cannot be parsed so the user can locate it easier. Follow up the error message with helpful instructions: they can resolve the error by fixing the date or excluding the file or directory from being processed.

Won't solve #1844 but will maybe help point in the right direction.

Better error message when time is not parseable
Give the full path of the file that cannot be parsed so the user can locate it easier. Follow up the error message with helpful instructions: they can resolve the error by fixing the date or excluding the file or directory from being processed.
@parkr

This comment has been minimized.

Member

parkr commented Dec 18, 2013

This is awesome! Thanks for taking the time to make up this PR.

Would you mind writing a quick test in test/test_post.rb which checks that this FatalException is raised?

@coveralls

This comment has been minimized.

coveralls commented Dec 18, 2013

Coverage Status

Coverage increased (+0.03%) when pulling 22017d0 on schneems:schneems/better-bat-time-error into 12a55b8 on jekyll:master.

@schneems

This comment has been minimized.

Contributor

schneems commented Dec 18, 2013

We're exercising that code here https://github.com/schneems/jekyll/blob/schneems/better-bat-time-error/test/test_post.rb?pr=%2Fjekyll%2Fjekyll%2Fpull%2F1847#L56-L58

This is mostly a messaging change, so the same error test should be sufficient.

On a side note, it looks like the \n in the exception doesn't quite work right, the whole thing comes out as one super long line for now:

Post '/vendor/jekyll-1.4.2/lib/site_template/0000-00-00-welcome-to-jekyll.markdown.erb' does not have a valid date. Fix the date, or exclude the file or directory from being processed
@schneems

This comment has been minimized.

Contributor

schneems commented Dec 22, 2013

Any additional comments here? Should I change something, or should we close this PR?

@parkr

This comment has been minimized.

Member

parkr commented Dec 22, 2013

Know why the exception string is being prevented from being multi-line? Does it break a spec for how exceptions should be presented?

Otherwise, looks awesome to me.

@@ -160,7 +160,10 @@ def process(name)
self.slug = slug
self.ext = ext
rescue ArgumentError
raise FatalException.new("Post #{name} does not have a valid date.")
path = File.join(@dir || "", name)

This comment has been minimized.

@parkr

parkr Dec 22, 2013

Member

Can we use #relative_path for this?

This comment has been minimized.

@schneems

schneems Dec 24, 2013

Contributor

Nope, when i change this we get a test failure:

[Jekyll::FatalException] exception expected, not
Class: <TypeError>
Message: <"no implicit conversion of nil into String">
---Backtrace---
/Users/schneems/Documents/projects/jekyll/lib/jekyll/post.rb:163:in `join'
/Users/schneems/Documents/projects/jekyll/lib/jekyll/post.rb:163:in `rescue in process'
/Users/schneems/Documents/projects/jekyll/lib/jekyll/post.rb:158:in `process'

Looks like @dir and @name can both be nil.

@parkr parkr merged commit 9d5785c into jekyll:master Dec 24, 2013

1 check passed

default The Travis CI build passed
Details

parkr added a commit that referenced this pull request Dec 24, 2013

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