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.

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

Colon silently made mandatory in 9.10.0 #208

Closed
pdl opened this issue Mar 24, 2020 · 5 comments
Closed

Colon silently made mandatory in 9.10.0 #208

pdl opened this issue Mar 24, 2020 · 5 comments

Comments

@pdl
Copy link

pdl commented Mar 24, 2020

It looks like the behaviour of filters has changed where there is no colon between a filter and its arguments between 9.9.0 and 9.10.0. I haven't spotted anything in the changes to indicate this was intentional. Where there is no colon after the filter name, arguments are ignored. If the original behaviour is now unsupported, I would have expected to see an error thrown.

echo '{{"world" | prepend "hello" }}' | npx [email protected]
helloworld

echo '{{"world" | prepend "hello" }}' | npx [email protected]
world
@harttle
Copy link
Owner

harttle commented Mar 24, 2020

Thank you for the work to find it out. This case is not covered before and I'm surprised to know it worked once upon a time.

If the original behaviour is now unsupported, I would have expected to see an error thrown.

You're right, it should be regarded invalid for prepend to have undefined to prepend. I'll throw something upon this.

@harttle harttle added the bug label Mar 24, 2020
harttle pushed a commit that referenced this issue Mar 24, 2020
## [9.11.5](v9.11.4...v9.11.5) (2020-03-24)

### Bug Fixes

* throws on invalid arguments for prepend/append, fixes [#208](#208) ([479c633](479c633))
@harttle
Copy link
Owner

harttle commented Mar 24, 2020

🎉 This issue has been resolved in version 9.11.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

@pdl
Copy link
Author

pdl commented Mar 24, 2020

I was actually expecting the compiler to throw a syntax error because of the extra tokens rather than just discard the tokens and call the function with no arguments. I picked prepend arbitrarily, but this approach would not help for e.g. round, which has an optional first argument:

echo '{{ 2.22222 | round 2 }}' | npx [email protected]
2.22

echo '{{ 2.22222 | round 2 }}' | npx [email protected]
2

I would be worried that anyone who has missed the colon might not notice that the argument has not been passed to round at all.

@harttle
Copy link
Owner

harttle commented Mar 24, 2020

It's intentional in Liquid to be safe and customer facing. I tried the Ruby version both the following renders to 3 (even with strict_filters=true which I guess throws only when filter not defined):

'{{ 3.456 | round }}'
'{{ 3.456 | round 2 }}'
'{{ 3.456 | round 2 a b c ? }}'

But for preppend, it seems that functions in Ruby won't tolerate wrong number of arguments, which in JavaScript is asserting in cases expecting an Error throwing.

@pdl
Copy link
Author

pdl commented Mar 24, 2020

I see! I guess it makes sense to be consistent. Thank you for confirming, and so quickly, too.

harttle pushed a commit that referenced this issue Mar 31, 2020
## [9.11.8](v9.11.7...v9.11.8) (2020-03-31)

### Bug Fixes

* throw an error if : omitted unintentionally, [#212](#212), [#208](#208) ([8daf281](8daf281))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants