set `LiquidError#template_name` for errors in included file #6206

Crunch09 · 2017-07-05T20:29:30Z

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 jekyll#6203

ashmaroli · 2017-07-06T06:34:54Z

lib/jekyll/tags/include.rb

-        super(msg)
-        @path = path
-      end
-    end



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

😞 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!

Some plugins use this class: https://github.com/search?l=Ruby&q=IncludeTagError+language%3ARuby&ref=advsearch&type=Code&utf8=%E2%9C%93

Thanks @DirtyF !

Whoa! That's a lot.. 😅

@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 · 2017-07-10T19:57:06Z

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

parkr

Love this change! Just 1 nit-pick. 😄

parkr · 2017-07-25T23:11:19Z

lib/jekyll/tags/include.rb

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


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

parkr · 2017-07-25T23:11:25Z

lib/jekyll/tags/include.rb

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


Same here with e.

Crunch09 · 2017-07-26T11:08:05Z

@parkr Thanks! Fixed now

parkr

2 more things! Sorry 😺

parkr · 2017-07-30T18:34:06Z

features/rendering.feature

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


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.

parkr · 2017-07-30T18:35:23Z

features/rendering.feature

+    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


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

Crunch09 · 2017-07-30T21:06:15Z

@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 · 2017-08-01T16:23:46Z

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 · 2017-08-01T16:30:21Z

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

ashmaroli · 2017-08-01T16:31:38Z

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 · 2017-08-01T16:34:23Z

@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 · 2017-08-01T16:41:22Z

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

parkr

😍 THIS IS GORGEOUS! Thank you, @Crunch09!

parkr · 2017-08-04T02:00:23Z

@jekyllbot: merge +minor

jekyllbot assigned jekyllbot and parkr Jul 5, 2017

ashmaroli reviewed Jul 6, 2017

View reviewed changes

bring back IncludeTagError class

ae69165

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

parkr requested changes Jul 25, 2017

View reviewed changes

parkr added this to the 3.6 milestone Jul 25, 2017

rename ex to e for consistency

fe131dd

parkr requested changes Jul 30, 2017

View reviewed changes

improve scenarios to include entire error message

b176878

parkr approved these changes Aug 1, 2017

View reviewed changes

fix scenario for older ruby versions

eaf26b3

parkr approved these changes Aug 4, 2017

View reviewed changes

jekyllbot merged commit cc1cb81 into jekyll:master Aug 4, 2017

jekyllbot added the enhancement label Aug 4, 2017

jekyllbot added a commit that referenced this pull request Aug 4, 2017

Update history to reflect merge of #6206 [ci skip]

828ee76

Crunch09 deleted the error-in-included-file branch August 4, 2017 08:47

jekyll locked and limited conversation to collaborators Jul 12, 2019

jekyllbot added the frozen-due-to-age label Jul 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

set `LiquidError#template_name` for errors in included file #6206

set `LiquidError#template_name` for errors in included file #6206

Crunch09 commented Jul 5, 2017

ashmaroli Jul 6, 2017

Crunch09 Jul 6, 2017

DirtyF Jul 6, 2017

Crunch09 Jul 6, 2017

ashmaroli Jul 6, 2017

DirtyF Jul 6, 2017

Crunch09 commented Jul 10, 2017

parkr left a comment

parkr Jul 25, 2017

parkr Jul 25, 2017

Crunch09 commented Jul 26, 2017

parkr left a comment

parkr Jul 30, 2017

parkr Jul 30, 2017

Crunch09 commented Jul 30, 2017 •

edited

parkr commented Aug 1, 2017

ashmaroli commented Aug 1, 2017

ashmaroli commented Aug 1, 2017

pathawks commented Aug 1, 2017

Crunch09 commented Aug 1, 2017

parkr left a comment

parkr commented Aug 4, 2017

set LiquidError#template_name for errors in included file #6206

set LiquidError#template_name for errors in included file #6206

Conversation

Crunch09 commented Jul 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Crunch09 commented Jul 10, 2017

parkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Crunch09 commented Jul 26, 2017

parkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Crunch09 commented Jul 30, 2017 • edited

parkr commented Aug 1, 2017

ashmaroli commented Aug 1, 2017

ashmaroli commented Aug 1, 2017

pathawks commented Aug 1, 2017

Crunch09 commented Aug 1, 2017

parkr left a comment

Choose a reason for hiding this comment

parkr commented Aug 4, 2017

set `LiquidError#template_name` for errors in included file #6206

set `LiquidError#template_name` for errors in included file #6206

Crunch09 commented Jul 30, 2017 •

edited