Extract include tag read file in a method #1490

Merged
merged 13 commits into from Sep 19, 2013

Projects

None yet

5 participants

@penibelst
Member

With that change people can easily hook into the file read procedure by inheritting from the Jekyll::Tags::IncludeTag class. For example to write a 5 lines plugin that remove trailing file whitespaces (related #1455).

@maul-esel
Contributor

By itself, this code change feels strange to me; but with the given background I see no purpose to deny it unless there's a more general hook solution ahead.

I would recommend to document the purpose of that method in the code though, otherwise it might easily get killed during a refactoring in the future.

@mattr-
Member
mattr- commented Sep 14, 2013

I think it might make more sense to refactor the source variable into a source method that does the same thing as your method. This also means that includes_dir might need to become a method as well. This would take care of two things at once: implement your use case and refactor this class a bit.

Would you give it a try and update your pull request?

@penibelst
Member

Yes, I’ll try it. Thank you for pointing me at the better solution.

@penibelst
Member

@mattr- Done.

@parkr parkr commented on an outdated diff Sep 15, 2013
lib/jekyll/tags/include.rb
@@ -4,6 +4,8 @@ class IncludeTag < Liquid::Tag
MATCHER = /([\w-]+)\s*=\s*(?:"([^"\\]*(?:\\.[^"\\]*)*)"|'([^'\\]*(?:\\.[^'\\]*)*)'|([\w\.-]+))/
+ INCLUDES = '_includes'
@parkr
parkr Sep 15, 2013 Member

INCLUDES_DIR would make more sense here.

@parkr parkr commented on an outdated diff Sep 15, 2013
lib/jekyll/tags/include.rb
@@ -62,22 +65,36 @@ def render(context)
end
end
- def validate_file(includes_dir)
- if File.symlink?(includes_dir)
- return "Includes directory '#{includes_dir}' cannot be a symlink"
+ def validate_file(dir)
+ if File.symlink?(dir)
@parkr
parkr Sep 15, 2013 Member

Let's make this conditional on the site being in safe mode so people can symlink it if they want in unsafe mode. Thoughts?

@parkr parkr commented on an outdated diff Sep 15, 2013
lib/jekyll/tags/include.rb
if !File.exists?(file)
- return "Included file #{@file} not found in _includes directory"
+ return "Included file #{@file} not found in #{INCLUDES} directory"
elsif File.symlink?(file)
@parkr
parkr Sep 15, 2013 Member

I also think the files could be symlinks in unsafe mode.

@parkr
Member
parkr commented Sep 15, 2013

Looks pretty good so far. @mattr- what do you think?

We'll also need a rebase please.

@penibelst
Member

Now the class looks really dry. I hope you guys like it.

@parkr parkr commented on an outdated diff Sep 17, 2013
lib/jekyll/tags/include.rb
@@ -4,6 +4,10 @@ class IncludeTag < Liquid::Tag
MATCHER = /([\w-]+)\s*=\s*(?:"([^"\\]*(?:\\.[^"\\]*)*)"|'([^'\\]*(?:\\.[^'\\]*)*)'|([\w\.-]+))/
+ VALID_SYNTAX = "{% include file.ext param='value' param2='value' %}"
@parkr
parkr Sep 17, 2013 Member

What do you think of SYNTAX_EXAMPLE for this?

@parkr parkr commented on an outdated diff Sep 17, 2013
lib/jekyll/tags/include.rb
@@ -4,6 +4,10 @@ class IncludeTag < Liquid::Tag
MATCHER = /([\w-]+)\s*=\s*(?:"([^"\\]*(?:\\.[^"\\]*)*)"|'([^'\\]*(?:\\.[^'\\]*)*)'|([\w\.-]+))/
@parkr
parkr Sep 17, 2013 Member

What do you think of VALID_SYNTAX for this?

@parkr parkr commented on an outdated diff Sep 17, 2013
lib/jekyll/tags/include.rb
if !File.exists?(file)
- return "Included file #{@file} not found in _includes directory"
- elsif File.symlink?(file)
- return "The included file '_includes/#{@file}' should not be a symlink"
+ return "Included file '#{@file}' not found in '#{INCLUDES_DIR}' directory"
+ elsif File.symlink?(file) && safe?
+ return "The included file '#{INCLUDES_DIR}/#{@file}' should not be a symlink"
@parkr
parkr Sep 17, 2013 Member

I'd prefer not to have the explicit returns here if that's ok.

@parkr parkr commented on an outdated diff Sep 17, 2013
lib/jekyll/tags/include.rb
@@ -64,26 +91,29 @@ def render(context)
end
end
- def validate_file(includes_dir)
- if File.symlink?(includes_dir)
- return "Includes directory '#{includes_dir}' cannot be a symlink"
- end
-
- if @file !~ /^[a-zA-Z0-9_\/\.-]+$/ || @file =~ /\.\// || @file =~ /\/\./
- return "Include file '#{@file}' contains invalid characters or sequences"
+ def validate_dir(dir, safe)
+ if File.symlink?(dir) && safe?
+ return "Includes directory '#{dir}' cannot be a symlink"
@parkr
parkr Sep 17, 2013 Member

No need for this return

@parkr parkr commented on an outdated diff Sep 17, 2013
lib/jekyll/tags/include.rb
@@ -64,26 +91,29 @@ def render(context)
end
end
- def validate_file(includes_dir)
- if File.symlink?(includes_dir)
- return "Includes directory '#{includes_dir}' cannot be a symlink"
- end
-
- if @file !~ /^[a-zA-Z0-9_\/\.-]+$/ || @file =~ /\.\// || @file =~ /\/\./
- return "Include file '#{@file}' contains invalid characters or sequences"
+ def validate_dir(dir, safe)
+ if File.symlink?(dir) && safe?
@parkr
parkr Sep 17, 2013 Member

I think this safe? is a CoffeeScript paradigm, but I don't think it's in Ruby.

@mattr- mattr- commented on an outdated diff Sep 17, 2013
lib/jekyll/tags/include.rb
end
end
def blank?
false
end
+
+ # This method allows to modify the file content by inheriting from the class.
+ # Don’t refactor it.
@mattr-
mattr- Sep 17, 2013 Member

I don't think this last line in the comment is necessary anymore. Because it's obvious how the method is used now, it's unlikely to be a candidate for a refactoring (and in fact looks like it was created as part of a refactoring) 😃

@mattr-
Member
mattr- commented Sep 17, 2013

Just a few small things and then this will be good to go. Nice work @penibelst ❤️

@coveralls

Coverage Status

Changes Unknown when pulling 25519b3 on penibelst:allow-include-read-override into * on mojombo:master*.

@coveralls

Coverage Status

Changes Unknown when pulling 7cec996 on penibelst:allow-include-read-override into * on mojombo:master*.

@maul-esel
Contributor

Might I note: early validation of file name etc. is, by itself, a very good idea. However, it is totally incompatible to #1495 (variables as file names). In case this PR is merged first, I'll have to rebase mine and partly revert this change.

@parkr parkr commented on an outdated diff Sep 17, 2013
lib/jekyll/tags/include.rb
@@ -31,10 +39,24 @@ def parse_params(context)
params
end
- # ensure the entire markup string from start to end is valid syntax, and params are separated by spaces
- def validate_syntax
- full_matcher = Regexp.compile('\A\s*(?:' + MATCHER.to_s + '(?=\s|\z)\s*)*\z')
- unless @params =~ full_matcher
+ def validate_file_name
+ if @file !~ /^[a-zA-Z0-9_\/\.-]+$/ || @file =~ /\.\// || @file =~ /\/\./
+ raise SyntaxError.new <<-eos
@parkr
parkr Sep 17, 2013 Member

Do you think this should be an ArgumentError? I think SyntaxError is reserved for Ruby syntax issues.

@parkr
Member
parkr commented Sep 17, 2013

@penibelst I really, really like this. Thank you so much for all your hard work on this!!

@coveralls

Coverage Status

Changes Unknown when pulling f97eed5 on penibelst:allow-include-read-override into * on mojombo:master*.

@penibelst
Member

@maul-esel Do what you have to do. All I initially wanted was to extract the source read procedure in a one line method.

@parkr Thank you.

What is the Invisible Man @coveralls trying to tell me?

@maul-esel
Contributor

@coveralls is the bot of coveralls.io, trying to determine if merging your PR improves the test suite (but failing, because it has been setup for jekyll after you opened your PR).

@penibelst
Member

I’ll block @coveralls as long as it clutters human comments.

@maul-esel
Contributor

Yeah I liked it at first as well, but it's a little annyoing now. The commenting can be turned off though on coveralls.io. Not sure if @parkr and @mattr- as collaborators can do it, or just @mojombo himself.

@parkr
Member
parkr commented Sep 18, 2013

I've turned it off for now. It's kind of helpful as we require tests for all contributions (per our CONTRIBUTING docs) but it does clutter for the push-frequent folks.

@parkr
Member
parkr commented Sep 18, 2013

This PR LGTM. @mattr-?

@mattr-
Member
mattr- commented Sep 19, 2013

❤️ ❤️ ❤️ ❤️ ❤️

Nice work!

@mattr- mattr- merged commit 793eb96 into jekyll:master Sep 19, 2013

1 check failed

default The Travis CI build failed
Details
@mattr- mattr- added a commit that referenced this pull request Sep 19, 2013
@mattr- mattr- Update history to reflect merge of #1490 7ccb62e
@penibelst penibelst deleted the unknown repository branch Sep 19, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment