Extract include tag read file in a method #1490
Conversation
|
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. |
|
I think it might make more sense to refactor the Would you give it a try and update your pull request? |
|
Yes, I’ll try it. Thank you for pointing me at the better solution. |
|
@mattr- Done. |
| @@ -4,6 +4,8 @@ class IncludeTag < Liquid::Tag | |||
|
|
|||
| MATCHER = /([\w-]+)\s*=\s*(?:"([^"\\]*(?:\\.[^"\\]*)*)"|'([^'\\]*(?:\\.[^'\\]*)*)'|([\w\.-]+))/ | |||
|
|
|||
| INCLUDES = '_includes' | |||
There was a problem hiding this comment.
INCLUDES_DIR would make more sense here.
|
Looks pretty good so far. @mattr- what do you think? We'll also need a rebase please. |
|
Now the class looks really dry. I hope you guys like it. |
| @@ -4,6 +4,10 @@ class IncludeTag < Liquid::Tag | |||
|
|
|||
| MATCHER = /([\w-]+)\s*=\s*(?:"([^"\\]*(?:\\.[^"\\]*)*)"|'([^'\\]*(?:\\.[^'\\]*)*)'|([\w\.-]+))/ | |||
|
|
|||
| VALID_SYNTAX = "{% include file.ext param='value' param2='value' %}" | |||
There was a problem hiding this comment.
What do you think of SYNTAX_EXAMPLE for this?
|
Just a few small things and then this will be good to go. Nice work @penibelst ❤️ |
|
Changes Unknown when pulling 25519b3 on penibelst:allow-include-read-override into * on mojombo:master*. |
|
Changes Unknown when pulling 7cec996 on penibelst:allow-include-read-override into * on mojombo:master*. |
|
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. |
| unless @params =~ full_matcher | ||
| def validate_file_name | ||
| if @file !~ /^[a-zA-Z0-9_\/\.-]+$/ || @file =~ /\.\// || @file =~ /\/\./ | ||
| raise SyntaxError.new <<-eos |
There was a problem hiding this comment.
Do you think this should be an ArgumentError? I think SyntaxError is reserved for Ruby syntax issues.
|
@penibelst I really, really like this. Thank you so much for all your hard work on this!! |
|
Changes Unknown when pulling f97eed5 on penibelst:allow-include-read-override into * on mojombo:master*. |
|
@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? |
|
@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). |
|
I’ll block @coveralls as long as it clutters human comments. |
|
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. |
|
This PR LGTM. @mattr-? |
|
❤️ ❤️ ❤️ ❤️ ❤️ Nice work! |
Extract include tag read file in a method
With that change people can easily hook into the file read procedure by inheritting from the
Jekyll::Tags::IncludeTagclass. For example to write a 5 lines plugin that remove trailing file whitespaces (related #1455).