[LiquidDoc] Parsing support for optional parameters and default values #753
Conversation
jamesmengo
force-pushed
the
jm/parse_optional_param_with_default
branch
2 times, most recently
from
0a52108 to
49fdddd
Compare
| @@ -0,0 +1,6 @@ | |||
| --- | |||
| '@shopify/liquid-html-parser': minor | |||
| 'theme-check-vscode': minor | |||
There was a problem hiding this comment.
not sure why this is being included - is anything going to break if I remove this? 🤔
There was a problem hiding this comment.
Since there are no changes to tcv in here, you don't have to. Might have a syntax change that isn't being committed?
I sometimes add it even without changes for changesets that should be public facing.
| @@ -1011,30 +1011,352 @@ describe('Unit: Stage 1 (CST)', () => { | |||
| expectPath(cst, '0.children.0.value').to.equal('@param'); | |||
| }); | |||
|
|
|||
| it('should parse @param with name', () => { | |||
| const testStr = `{% doc %} @param paramWithNoDescription {% enddoc %}`; | |||
| it('should parse required @param', () => { | |||
There was a problem hiding this comment.
In these tests, I typically test permutations, and include location assertions for the first permutation only (aside from the whitespace case).
To avoid being overly verbose / repetitive, I'm just checking the value of the description / type to ensure that nothing has completely broken in the matching logic.
| paramDescription: ConcreteTextNode | null; | ||
| paramType: ConcreteTextNode | null; | ||
| } | ||
|
|
||
| export interface ConcreteLiquidDocParamNameNode |
There was a problem hiding this comment.
The syntax for default value and required properties are related to how the ParamName is defined, so I'm choosing to include that information in this ConcreteLiquidDocParamNameNode
This makes it a bit funky to access, but this aligns with how the parsing logic works, and allows us to leverage the Mappings mechanism. We map this to a more sensible location in stage-2 (node.paramName.required -> node.required)
---- - Added a parameter `required` to the LiquidDocParamNode - By default, parameters are required unless they have `[]` around the name - Parameters with incomplete delimiters `e.g. ([missingTail)` will map to a `TextNode` - Default values are supported for optional parameters - `[param=default]` - Default values will match anything between `=` and `]`. Leading and Trailing spaces are trimmed. I originally tried accessing everything from the top level in the param node. This is still the goal, but I think this should be done at stage 2 so that we can leverage the recursive nature of how to-AST works. My original approach attempted to Write everything to the top level in stage 1, but we should be doing this in stage 2.
jamesmengo
force-pushed
the
jm/parse_optional_param_with_default
branch
from
49fdddd to
463c120
Compare
| position: position(node.paramName), | ||
| source: node.paramName.source, | ||
| }, | ||
| paramName: toTextNode(node.paramName.paramNameContent), |
There was a problem hiding this comment.
@param {String} [param_name=123]
This allows us to refer to the node.paramName, which will be the node referring to param_name with the associated location info
| export interface ConcreteLiquidDocParamNameNode | ||
| extends ConcreteBasicNode<ConcreteNodeTypes.LiquidDocParamNameNode> { | ||
| required: boolean; | ||
| defaultValue: ConcreteTextNode | null; |
There was a problem hiding this comment.
I'm not sure we should support default value as it would do nothing unless you also add the {% assign x | x | default %} ?
|
We've decided to go in a different direction at this time. |
What are you adding in this PR?
Closes 533
Closes 534
Adds
LiquidDocparsing support to allow users to define optional parameters and default values.Optional Parameters
[]around the namee.g. ([missingTail)will map to aTextNodeDefault Values
[param=default]=and]. Leading and Trailing spaces are trimmed.What's next? Any followup issues?
#734
Before you deploy
changeset