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.

Sign up for GitHub

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

Jump to bottom

set LiquidError#template_name for errors in included file #6206

Merged
merged 5 commits into from Aug 4, 2017
Merged

set LiquidError#template_name for errors in included file #6206

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c580b4a
set `LiquidError#template_name` for errors in included file
Crunch09 Jul 5, 2017
ae69165
bring back `IncludeTagError` class
Crunch09 Jul 10, 2017
fe131dd
rename `ex` to `e` for consistency
Crunch09 Jul 26, 2017
b176878
improve scenarios to include entire error message
Crunch09 Jul 30, 2017
eaf26b3
fix scenario for older ruby versions
Crunch09 Aug 1, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 10 additions & 1 deletion features/rendering.feature
View file
Open in desktop
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


Scenario: When receiving Liquid with incorrect syntax in included file
Given I have a _includes directory
And I have a "_includes/invalid.html" file that contains "{{ site.title | prepend 'Prepended Text' }}"
And I have a "index.html" page with layout "simple" that contains "{% include invalid.html %}"
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


Scenario: Render Liquid and place in layout
Given I have a "index.html" page with layout "simple" that contains "Hi there, Jekyll {{ jekyll.environment }}!"
Expand Down
3 changes: 0 additions & 3 deletions lib/jekyll/liquid_renderer.rb
View file
Open in desktop
Expand Up @@ -41,9 +41,6 @@ def stats_table(n = 50)
end

def self.format_error(e, path)
if e.is_a? Tags::IncludeTagError
return "#{e.message} in #{e.path}, included in #{path}"
end
"#{e.message} in #{path}"
end
end
Expand Down
22 changes: 11 additions & 11 deletions lib/jekyll/tags/include.rb
View file
Open in desktop
Expand Up @@ -2,14 +2,6 @@

module Jekyll
module Tags
class IncludeTagError < StandardError
attr_accessor :path

def initialize(msg, path)
super(msg)
@path = path
end
end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @DirtyF !

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoa! That's a lot.. 😅

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

class IncludeTag < Liquid::Tag
VALID_SYNTAX = %r!
Expand Down Expand Up @@ -135,7 +127,13 @@ def render(context)

context.stack do
context["include"] = parse_params(context) if @params
partial.render!(context)
begin
partial.render!(context)
rescue Liquid::Error => ex
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ex.template_name = path
ex.markup_context = "included " if ex.markup_context.nil?
raise ex
end
end
end

Expand All @@ -160,8 +158,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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here with e.

ex.template_name = path
ex.markup_context = "included " if ex.markup_context.nil?
raise ex
end
end
end
Expand Down