Add completion support for filter parameters #522
Conversation
graygilmore
force-pushed
the
gg-complete-filter-arguments
branch
from
b13994b
to
32405cf
Compare
| @@ -489,16 +494,19 @@ StrictLiquidHTML <: LiquidHTML { | |||
| } | |||
|
|
|||
| WithPlaceholderLiquid <: Liquid { | |||
| liquidFilter<delim> := space* "|" space* identifier (space* ":" space* filterArguments<delim>)? | |||
There was a problem hiding this comment.
We only want this behavior for the placeholder modes to assist with autocompletion.
| const currentContext = params.completionContext.ancestors.at(-1); | ||
| // We don't want to show generic variable completions when we're inside a | ||
| // filter. This case will be handled by FilterNamedParameterCompletionProvider. | ||
| if (currentContext?.type === NodeTypes.LiquidFilter) { |
There was a problem hiding this comment.
Since the incomplete filter statement ends with LiquidVariableOutput we need to skip the lookup here if we're inside a filter or else we'll show all possible autocomplete values.
There was a problem hiding this comment.
I think this is fine for positional arguments, but isn't fine for when there were NamedArguments in the args array of the filter ancestor?
in other words:
- We do want completion of variables if the variable lookup could be a positional argument
- We don't want completion of variables if there were named arguments before the lookup being completed
There was a problem hiding this comment.
We probably could use some kind of sortText CompletionItem stuff to make sure that the properties are shown before the variables?
There was a problem hiding this comment.
💯 let me make that change.
| for (const { toCST, expectPath } of testCases) { | ||
| cst = toCST(`{{ a[1].foo | filtername: 10, myvar.foo, a1: 2, a2 }}`); | ||
| expectPath(cst, '0.type').to.equal('LiquidVariableOutput'); | ||
| } |
There was a problem hiding this comment.
Shouldn't this only pass in placeholder mode? 🤔
I think you do that with toCST(..., { mode: 'completion' })
There was a problem hiding this comment.
Oh I think we're just not going deep enough on the expectPath here.
| }); | ||
|
|
||
| return options.map(({ entry, types }) => | ||
| createCompletionItem( |
There was a problem hiding this comment.
I noticed that when you have you cursor in the middle of the partial, you don't replace the entire word. I got around it by finding the existing length and doing a TextEdit.replace (example). Should we consider that here too @charlespwd ?
E.g.
{{ 'base.css' | image_tag: he█ight }} ===> {{ 'base.css' | image_tag: heightight }}
There was a problem hiding this comment.
Interesting! Good call out. That's annoying to have to handle but makes sense in terms of total completeness of this (and matching other behavior).
|
|
||
| return options.map(({ entry, types }) => | ||
| createCompletionItem( | ||
| entry, |
There was a problem hiding this comment.
Is there any limitations today that prevent us from putting a ":" after the param? It would be great if we can do something like ${param}: $0.
There was a problem hiding this comment.
There is! Not all params are named and will require a :. I'm working on updating the source of the docs to include this so that we can do that!
There was a problem hiding this comment.
Perfect ❤️ In this PR or a follow-up one? 😅
There was a problem hiding this comment.
It'll be in a totally separate repo to build out support for this. I'll switch this one back to draft in the mean time!
graygilmore
force-pushed
the
gg-complete-filter-arguments
branch
2 times, most recently
from
8cc9093
to
67cf286
Compare
We want to add autocomplete behavior for the case where a developer is
typing out a liquid statement but isn't finished yet:
```liquid
{{ product | image_url: █
```
The scenario above works without any extra code but if we were to do:
```liquid
{{ product | image_url: width: 100, h█
```
Our parser would blow up because this isn't valid liquid anymore. It
interprets the `h` as a positional argument which cannot come after a
named argument. To support this we need to override how the grammar for
liquid filters is defined in the Placeholder scenario to support this.
The changes will treat the `h` as a `LiquidVariableOutput` and include
proper hierarchy.
The theme doc generator has been updated to include this attribute so that we can add better support for providing completions for filters.
graygilmore
force-pushed
the
gg-complete-filter-arguments
branch
from
67cf286
to
6c38ea7
Compare
graygilmore
force-pushed
the
gg-complete-filter-arguments
branch
from
6c38ea7
to
4853983
Compare
What are you adding in this PR?
Adds support for completion options for filter parameters. E.g.
{{ product | image_url: █ }}Will show completion options for
image_urlparameters!I've split the PR into two commits:
Before you deploy
yarn buildand committed the updated configuration fileschangeset