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

Fix include tag to avoid NameError #1494

Merged
merged 1 commit into from Sep 5, 2013

Conversation

Projects
None yet
5 participants
@maul-esel
Contributor

maul-esel commented Sep 2, 2013

This fixes a bug that is a result of my refactoring done in #1341: when the file name fails to validate, instead of rendering the error message to the file, a NameError exception is raised because the variable error is not properly initialized.

@@ -51,7 +51,9 @@ def validate_syntax
def render(context)
includes_dir = File.join(context.registers[:site].source, '_includes')
return error if error = validate_file(includes_dir)
if error = validate_file(includes_dir)
return error

This comment has been minimized.

@maul-esel

maul-esel Sep 2, 2013

Contributor

If we were determined to stay inline, one could also use the following code:

return error ||= "" if error = validate_file(includes_dir)

I didn't chose that though because it looks most confusing to me.

This comment has been minimized.

@parkr

parkr Sep 4, 2013

Member

Idea: what if you did this?

validate_file(includes_dir) || begin
   # ... everything else
end

This comment has been minimized.

@maul-esel

maul-esel Sep 5, 2013

Contributor

Another possible solution, but (at least to me as non-rubyist) just as cryptic if not worse. What's wrong with the if-block?

This comment has been minimized.

@parkr

parkr Sep 5, 2013

Member

It's not quite as idiomatic. Perhaps this?

validate_file(includes_dir) || fetch_include_contents(includes_dir)
@maul-esel

This comment has been minimized.

Contributor

maul-esel commented Sep 2, 2013

If, however, you're planning on supporting variable includes soon, #1495 also solves this problem.

@doktorbro

This comment has been minimized.

Member

doktorbro commented Sep 2, 2013

@maul-esel Do you think you could introduce my proposal #1490 together with all your refactorings in the IncludeTag class?

@maul-esel

This comment has been minimized.

Contributor

maul-esel commented Sep 2, 2013

I probably could, but what for? It's a valid suggestion worth of discussion, you already have a PR ready, and it might be best to let them be separate, so each one can be discussed and merged or rejected independent from the other. They shouldn't even conflict.

@doktorbro

This comment has been minimized.

Member

doktorbro commented Sep 2, 2013

@maul-esel Forgive my impatience. You are right.

@parkr

This comment has been minimized.

Member

parkr commented Sep 3, 2013

Can you paste the NameError output you get (with --trace)?

@maul-esel

This comment has been minimized.

Contributor

maul-esel commented Sep 3, 2013

Here it is:

Liquid Exception: undefined local variable or method `error' for #<Jekyll::Tags::IncludeTag:0x000000011479f8> in index.html
Dev/jekyll/lib/jekyll/tags/include.rb:54:in `render': undefined local variable or method `error' for #<Jekyll::Tags::IncludeTag:0x000000011479f8> (NameError)
    from /var/lib/gems/1.9.1/gems/liquid-2.5.1/lib/liquid/block.rb:106:in `block in render_all'
    from /var/lib/gems/1.9.1/gems/liquid-2.5.1/lib/liquid/block.rb:93:in `each'
    from /var/lib/gems/1.9.1/gems/liquid-2.5.1/lib/liquid/block.rb:93:in `render_all'
    from /var/lib/gems/1.9.1/gems/liquid-2.5.1/lib/liquid/block.rb:82:in `render'
    from /var/lib/gems/1.9.1/gems/liquid-2.5.1/lib/liquid/template.rb:124:in `render'
    from /var/lib/gems/1.9.1/gems/liquid-2.5.1/lib/liquid/template.rb:132:in `render!'
    from /home/dominik/Dev/jekyll/lib/jekyll/convertible.rb:81:in `render_liquid'
    from /home/dominik/Dev/jekyll/lib/jekyll/convertible.rb:139:in `do_layout'
    from /home/dominik/Dev/jekyll/lib/jekyll/page.rb:115:in `render'
    from /home/dominik/Dev/jekyll/lib/jekyll/site.rb:210:in `block in render'
    from /home/dominik/Dev/jekyll/lib/jekyll/site.rb:208:in `each'
    from /home/dominik/Dev/jekyll/lib/jekyll/site.rb:208:in `render'
    from /home/dominik/Dev/jekyll/lib/jekyll/site.rb:36:in `process'
    from /home/dominik/Dev/jekyll/lib/jekyll/command.rb:18:in `process_site'
    from /home/dominik/Dev/jekyll/lib/jekyll/commands/build.rb:23:in `build'
    from /home/dominik/Dev/jekyll/lib/jekyll/commands/build.rb:7:in `process'
    from ../bin/jekyll:72:in `block (2 levels) in <main>'
    from /var/lib/gems/1.9.1/gems/commander-4.1.4/lib/commander/command.rb:180:in `call'
    from /var/lib/gems/1.9.1/gems/commander-4.1.4/lib/commander/command.rb:180:in `call'
    from /var/lib/gems/1.9.1/gems/commander-4.1.4/lib/commander/command.rb:155:in `run'
    from /var/lib/gems/1.9.1/gems/commander-4.1.4/lib/commander/runner.rb:402:in `run_active_command'
    from /var/lib/gems/1.9.1/gems/commander-4.1.4/lib/commander/runner.rb:78:in `run!'
    from /var/lib/gems/1.9.1/gems/commander-4.1.4/lib/commander/delegates.rb:11:in `run!'
    from /var/lib/gems/1.9.1/gems/commander-4.1.4/lib/commander/import.rb:10:in `block in <top (required)>'

mattr- added a commit that referenced this pull request Sep 5, 2013

@mattr- mattr- merged commit 47f7832 into jekyll:master Sep 5, 2013

1 check passed

default The Travis CI build passed
Details

mattr- added a commit that referenced this pull request Sep 5, 2013

@maul-esel maul-esel deleted the maul-esel:fix-include-error branch Sep 7, 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.