Skip to content
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 `LiquidError#template_name` for errors in included file #6206

Merged
merged 5 commits into from Aug 4, 2017

Conversation

@Crunch09
Copy link
Member

Crunch09 commented Jul 5, 2017

Since Liquid 4 we can set the template_name of a Liquid::Error. In addition to that we also specify in the markup_context that this error occured in an included file.
This removes the need for a custom IncludeTagError

fixes #6203

Before (1):

Liquid Exception: Liquid syntax error (line 1): Unknown tag 'INVALID' in /Users/crunch/Code/jekyll/tmp/jekyll/_includes/invalid.html, included in index.html

After (1):

Liquid Exception: Liquid syntax error (/Users/crunch/Code/jekyll/tmp/jekyll/_includes/invalid.html line 1): Unknown tag 'INVALID' included in index.html

Before (2):

Liquid Exception: Liquid error (line 1): wrong number of arguments (given 1, expected 2) in index.html

After (2):

Liquid Exception: Liquid error (/Users/crunch/Code/jekyll/tmp/jekyll/_includes/invalid.html line 1): wrong number of arguments (given 1, expected 2) included in index.html

cc: @jekyll/stability

Since Liquid 4 we can set the `template_name` of a `Liquid::Error`.
In addition to that we also specify in the `markup_context` that this
error occured in an `included` file.
This removes the need for a custom `IncludeTagError`

fixes #6203
super(msg)
@path = path
end
end

This comment has been minimized.

Copy link
@ashmaroli

ashmaroli Jul 6, 2017

Member

Will this be a breaking change in a scenario where somebody wrote a plugin using this class? (Jekyll::Tags::IncludeTagError)

This comment has been minimized.

Copy link
@Crunch09

Crunch09 Jul 6, 2017

Author Member

😞 You're right. I think this case is unlikely but nonetheless we shouldn't remove this then. I'll update the PR later, thank you!

This comment has been minimized.

This comment has been minimized.

Copy link
@Crunch09

Crunch09 Jul 6, 2017

Author Member

Thanks @DirtyF !

This comment has been minimized.

Copy link
@ashmaroli

ashmaroli Jul 6, 2017

Member

Whoa! That's a lot.. 😅

This comment has been minimized.

Copy link
@DirtyF

DirtyF Jul 6, 2017

Member

@ashmaroli a lot of these are in fact Jekyll's own files as some people version their .bundle repository 😲

Plugins are relying on this class so we can't just remove it
@Crunch09

This comment has been minimized.

Copy link
Member Author

Crunch09 commented Jul 10, 2017

Sorry for the delay, but brought the IncludeTagError class back now. Should we deprecate it?

Copy link
Member

parkr left a comment

Love this change! Just 1 nit-pick. 😄

partial.render!(context)
begin
partial.render!(context)
rescue Liquid::Error => ex

This comment has been minimized.

Copy link
@parkr

parkr Jul 25, 2017

Member

Let's call this variable e for convention's sake.

@@ -160,8 +166,10 @@ def load_cached_partial(path, context)
.file(path)
begin
cached_partial[path] = unparsed_file.parse(read_file(path, context))
rescue Liquid::SyntaxError => ex
raise IncludeTagError.new(ex.message, path)
rescue Liquid::Error => ex

This comment has been minimized.

Copy link
@parkr

parkr Jul 25, 2017

Member

Same here with e.

@parkr parkr added this to the 3.6 milestone Jul 25, 2017
@Crunch09

This comment has been minimized.

Copy link
Member Author

Crunch09 commented Jul 26, 2017

@parkr Thanks! Fixed now

Copy link
Member

parkr left a comment

2 more things! Sorry 😺

@@ -19,7 +19,16 @@ Feature: Rendering
And I have a simple layout that contains "{{ content }}"
When I run jekyll build
Then I should get a non-zero exit-status
And I should see "Liquid Exception.*Unknown tag 'INVALID' in.*_includes/invalid\.html" in the build output
And I should see "Liquid Exception.+_includes/invalid\.html.+included in index\.html" in the build output

This comment has been minimized.

Copy link
@parkr

parkr Jul 30, 2017

Member

I'd like to be able to test that Unknown tag 'INVALID' is still in the output – would that be possible? It can be another line (And I should see...) if the regexp is getting to weird.

And I have a simple layout that contains "{{ content }}"
When I run jekyll build
Then I should get a non-zero exit-status
And I should see "Liquid Exception.+_includes/invalid\.html.+included in index\.html" in the build output

This comment has been minimized.

Copy link
@parkr

parkr Jul 30, 2017

Member

Can we also test the error message this outputs, rather than just the paths?

@Crunch09

This comment has been minimized.

Copy link
Member Author

Crunch09 commented Jul 30, 2017

@parkr No worries, all very good points! 😄 The scenarios now include the entire error message. Although the lines are quite long now, i kept each to one line as i think it's more readable than with two And i should see... and also the regex is not complicated.

@parkr
parkr approved these changes Aug 1, 2017
@parkr

This comment has been minimized.

Copy link
Member

parkr commented Aug 1, 2017

I restarted 2 failing cucumber scenarios in Travis. If those pass, then we're good to go. Please don't merge before then.

@pathawks, want to take a look here? 👀

@ashmaroli

This comment has been minimized.

Copy link
Member

ashmaroli commented Aug 1, 2017

@parkr Those cucumber tests wont pass because they're testing for a different string in Ruby 2.1.0

@ashmaroli

This comment has been minimized.

Copy link
Member

ashmaroli commented Aug 1, 2017

Ruby 2.1.0 raises:

ArgumentError: wrong number of arguments (2 for 1)

Ruby 2.3.0 raises:

ArgumentError: wrong number of arguments (given 2, expected 1)
@pathawks

This comment has been minimized.

Copy link
Member

pathawks commented Aug 1, 2017

@ashmaroli Yup.

- Liquid Exception: Liquid error (/home/travis/build/jekyll/jekyll/tmp/jekyll/_includes/invalid.html line 1): wrong number of arguments (given 1, expected 2) included in index.html
+ Liquid Exception: Liquid error (/home/travis/build/jekyll/jekyll/tmp/jekyll/_includes/invalid.html line 1): wrong number of arguments (1 for 2) included in index.html
@Crunch09

This comment has been minimized.

Copy link
Member Author

Crunch09 commented Aug 1, 2017

Good find @ashmaroli 👍 (and i should have paid attention to travis after submitting the latest update). I'm gonna fix those later today.

@parkr
parkr approved these changes Aug 4, 2017
Copy link
Member

parkr left a comment

😍 THIS IS GORGEOUS! Thank you, @Crunch09!

@parkr

This comment has been minimized.

Copy link
Member

parkr commented Aug 4, 2017

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit cc1cb81 into jekyll:master Aug 4, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jekyllbot added a commit that referenced this pull request Aug 4, 2017
@Crunch09 Crunch09 deleted the Crunch09:error-in-included-file branch Aug 4, 2017
@jekyll jekyll locked and limited conversation to collaborators Jul 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.