Consistent error handling in Liquid tags #1514

Merged
merged 5 commits into from Oct 1, 2013

Projects

None yet

5 participants

@maul-esel
Contributor

As discussed in #1496, this PR makes the error handling across the different custom Liquid tags in jekyll more consistent.

@maul-esel maul-esel commented on an outdated diff Sep 10, 2013
lib/jekyll/tags/include.rb
@@ -66,18 +64,18 @@ def render(context)
def validate_file(includes_dir)
if File.symlink?(includes_dir)
- return "Includes directory '#{includes_dir}' cannot be a symlink"
+ raise IOError.new "Includes directory '#{includes_dir}' cannot be a symlink"
@maul-esel
maul-esel Sep 10, 2013 Contributor

Is IOError a good idea in here? Or what else should be raised?

@maul-esel
Contributor

@parkr / @mattr- I fixed the tests failure, unless you want me to change the exception types (see comments above), this should be ready to 🚢

@parkr
Member
parkr commented Sep 12, 2013

👍

@penibelst
Member

As I worked at #1490 I was confused by the inconsistent error handling too. Why don’t you want to use original Liquid module exceptions?

module Liquid
  class Error < ::StandardError; end

  class ArgumentError < Error; end
  class ContextError < Error; end
  class FilterNotFound < Error; end
  class FileSystemError < Error; end
  class StandardError < Error; end
  class SyntaxError < Error; end
  class StackLevelError < Error; end
  class MemoryError < Error; end
end

I would say IOError is a FileSystemError and so on.

@maul-esel maul-esel commented on the diff Sep 19, 2013
lib/jekyll/tags/include.rb
file = File.join(dir, @file)
- if error = validate_file(dir, context.registers[:site].safe)
- return error
- end
+ validate_file(file, context.registers[:site].safe)
@maul-esel
maul-esel Sep 19, 2013 Contributor

Fixed a minor typo by @penibelst here - must be file, not dir.

@penibelst
penibelst Sep 20, 2013 Member

Nice catch.

@maul-esel
Contributor

Just rebased it once again.

About other exceptions: Could do that, but where's the advantage? Can we even rely upon liquid not to rename or remove its internal exception classes in future versions?

@maul-esel maul-esel and 1 other commented on an outdated diff Sep 19, 2013
lib/jekyll/tags/gist.rb
@@ -12,7 +12,15 @@ def render(context)
gist_id, filename = tag_contents[0], tag_contents[1]
gist_script_tag(gist_id, filename)
else
- "Error parsing gist id"
+ raise SyntaxError.new <<-eos
@maul-esel
maul-esel Sep 19, 2013 Contributor

Do you want me to change this to ArgumentError as well?

@parkr
parkr Sep 19, 2013 Member

Yes please :)

@maul-esel
maul-esel Sep 19, 2013 Contributor

Done! Anything else to do before 🚢ing?

@maul-esel maul-esel 'gist' tag: switch to ArgumentError exception class
SyntaxError is reserved for Ruby's internal use.

Adjust the tests, including the call to liquid to
make it rethrow ArgumentErrors.
1829c27
@parkr
Member
parkr commented Sep 20, 2013

BOOM. Looks pretty freakin good to me. @mattr-?

@penibelst
Member

About other exceptions: Could do that, but where's the advantage? Can we even rely upon liquid not to rename or remove its internal exception classes in future versions?

After recognizing that many of Liquid tag exceptions hold same names as Ruby core exceptions I don’t see any advantage.

@maul-esel maul-esel referenced this pull request Sep 29, 2013
Merged

Variable {% include %} #1495

4 of 4 tasks complete
@mattr- mattr- merged commit 9d4f916 into jekyll:master Oct 1, 2013

1 check passed

default The Travis CI build passed
Details
@mattr- mattr- added a commit that referenced this pull request Oct 1, 2013
@mattr- mattr- Update history to reflect merge of #1514 cb8572f
@maul-esel maul-esel deleted the maul-esel:tag-errors branch Oct 1, 2013
@benbalter
Contributor

Is this path relative to root or absolute path?

@parkr
Member
parkr commented Nov 2, 2013

@benbalter Can you please specify what you mean by "this"?

@benbalter
Contributor

Ha. That'd help. The file path in the error output. 😄

@parkr
Member
parkr commented Nov 2, 2013

@benbalter The only spot I see (the comments make it a bit harder to read) is the output in the Include tag. Those are all relative, such that {% include my_dir/file.html %} where @file is my_dir/file.html. So relative?

@benbalter
Contributor

🤘 thanks.

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