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

Add keyword arguments for Liquid shortcodes. #1269

Closed
wants to merge 3 commits into from

Conversation

dawaltconley
Copy link

@dawaltconley dawaltconley commented Jun 19, 2020

Fixes #1263. This removes moo and switches the argument lexer/parser for Liquid shortcodes to liquid-args, to support keyword arguments. Grammar is here but basically the differences are:

  1. Whitespace / arg separators are /,?[ \t\n\r]+/ instead of /[, \t]+/. So arg1 ,arg2 or arg1,,,arg2 would be invalid.
  2. Numbers only allow one dot: /[0-9]+\.?[0-9]*/ instead of /[0-9]+\.*[0-9]*/
  3. Variables can only have periods separating valid variable names, i.e foo.bar.baz and not ..foobar.baz.

1 could be breaking if people are getting freaky with their commas (and I'm not wed to it, could keep as is). The differences in 2 & 3 should only throw errors in cases that would also cause a Liquid rendering error.

I added some (possibly too many) tests as well, basically copying old tests and updating them to use keywords.

@MadeByMike
Copy link
Contributor

@dawaltconley this feature looks great, and you did a really good job with liquid-args. One question I have is why did you choose to write something new rather than use moo? Would replacing it be as well tested and benchmarked?

The breaking changes seem sensible. Do you think you could write some examples in terms of what's changed for consumers? Particularly if breaking E.g.:

Previously, the following arguments would be parsed:

{% shortcode arg1, , arg2 %}

{% shortcode arg1 ,arg2 %}

Now, argument parsing is more strict and this should be changed to:

{% shortcode arg1, arg2 %}

Etc...

@dawaltconley
Copy link
Author

Honestly, I wrote it for myself and was halfway done by the time I thought of integrating it into 11ty. :P Otherwise I might have tried to figure it out with moo.

I'm more familiar with PEG.js and probably not the best person to tackle it with moo but would be down to try if y'all think that's better. I assume any change to the current lexer will be only as well-tested as the 11ty tests allow, but I should probably write more tests for liquid-args anyway, including testing for expected failures.

And yes, down to write up the breaking changes, should have time this week. Currently this would also break:

{% shortcode arg1,arg2,arg3 %}

And the parser should probably be able to handle that at least. Will update.

@zachleat
Copy link
Member

Sorry for the late reply here.

Definitely a fan of this and seems like it might be a good fit for 1.0 as long as the breaking changes aren’t too drastic.

I am worried about arg1,arg2 specifically but not so much arg1 ,arg2 or arg1,,arg2

@zachleat zachleat added the open-question Requires additional information before we can proceed. label Mar 18, 2021
@zachleat
Copy link
Member

Howdy y’all—still interested in this one!

@dawaltconley
Copy link
Author

dawaltconley commented Apr 9, 2021

Howdy thanks for the bump and sorry for the delay on this! Going back to it it looks like I fixed the white space issue, so commas with no spaces a la {% shortcode arg1,arg2,arg3 %} will work now.

It's pretty arbitrary; it parses white space as [, \t\n\r]+ and then throws an error if any character other than the first is a comma. But I'll keep it that way if the other breaking changes are okay, since it's closer to Liquid's behavior.

Will submit a new pull request with details re breaking changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-question Requires additional information before we can proceed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support keyword arguments for Liquid shortcodes.
3 participants