Skip to content

Add keyword arguments for Liquid shortcodes.#1269

Closed
dawaltconley wants to merge 3 commits into11ty:masterfrom
dawaltconley:master
Closed

Add keyword arguments for Liquid shortcodes.#1269
dawaltconley wants to merge 3 commits into11ty:masterfrom
dawaltconley:master

Conversation

@dawaltconley
Copy link
Copy Markdown

@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

Howdy y’all—still interested in this one!

@dawaltconley
Copy link
Copy Markdown
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.

Development

Successfully merging this pull request may close these issues.

Support keyword arguments for Liquid shortcodes.

3 participants