Merge pull request #522 from Shopify/gg-complete-filter-arguments

Add completion support for filter parameters

.changeset/dull-plants-shake.md

@@ -0,0 +1,7 @@
+---
+'@shopify/theme-language-server-common': patch
+---
+
+Add filter completion support
+
+We'll now provide completions for Liquid filter parameters when requested.

.changeset/metal-seahorses-breathe.md

@@ -0,0 +1,5 @@
+---
+'@shopify/theme-language-server-common': patch
+---
+
+Fix a bug where filter completions wouldn't replace existing text

.changeset/new-peas-attack.md

@@ -0,0 +1,10 @@
+---
+'@shopify/theme-language-server-common': patch
+---
+
+Add required parameters when autocompleting filters
+
+When autocompleting a Liquid filter that has required parameters we'll
+automatically add them with tab support. For example, if you using the
+completion feature to add the `image_url` filter we'll automatically add the
+`width` and `height` parameters for it.

.changeset/pretty-maps-fix.md

@@ -0,0 +1,15 @@
+---
+'@shopify/liquid-html-parser': patch
+---
+
+Add support for incomplete filter statements
+
+Adds the ability to continue to parse liquid code that would otherwise be
+considered invalid:
+
+```liquid
+{{ product | image_url: width: 100, c█ }}
+```
+
+The above liquid statement will now successfully parse in all placeholder modes
+so that we can offer completion options for filter parameters.

.changeset/unlucky-carpets-talk.md

@@ -0,0 +1,5 @@
+---
+'@shopify/theme-check-common': patch
+---
+
+Add positional attribute to exported Parameter type

packages/liquid-html-parser/grammar/liquid-html.ohm

@@ -264,6 +264,11 @@ Liquid <: Helpers {
   positionalArgument<delim> = liquidExpression<delim> ~(space* ":")
   namedArgument<delim> = variableSegment space* ":" space* liquidExpression<delim>
   tagArguments = listOf<namedArgument<delimTag>, argumentSeparatorOptionalComma>
+  filterArguments<delim> =
+    | complexArguments<delim>
+    | simpleArgument<delim>
+  complexArguments<delim> = arguments<delim> (space* "," space* simpleArgument<delim>)?
+  simpleArgument<delim> = liquidVariableLookup<delim>
 
   variableSegment = (letter | "_") (~endOfTagName identifierCharacter)*
   variableSegmentAsLookup = variableSegment
@@ -495,18 +500,21 @@ StrictLiquidHTML <: LiquidHTML {
 }
 
 WithPlaceholderLiquid <: Liquid {
+  liquidFilter<delim> := space* "|" space* identifier (space* ":" space* filterArguments<delim> (space* ",")?)?
   liquidTagName := (letter | "█") (alnum | "_")*
-  variableSegment := (letter | "_" | "█") identifierCharacter*
+  variableSegment := (letter | "_" | "█") (identifierCharacter | "█")*
 }
 
 WithPlaceholderLiquidStatement <: LiquidStatement {
+  liquidFilter<delim> := space* "|" space* identifier (space* ":" space* filterArguments<delim> (space* ",")?)?
   liquidTagName := (letter | "█") (alnum | "_")*
-  variableSegment := (letter | "_" | "█") identifierCharacter*
+  variableSegment := (letter | "_" | "█") (identifierCharacter | "█")*
 }
 
 WithPlaceholderLiquidHTML <: LiquidHTML {
+  liquidFilter<delim> := space* "|" space* identifier (space* ":" space* filterArguments<delim> (space* ",")?)?
   liquidTagName := (letter | "█") (alnum | "_")*
-  variableSegment := (letter | "_" | "█") identifierCharacter*
+  variableSegment := (letter | "_" | "█") (identifierCharacter | "█")*
   leadingTagNameTextNode := (letter | "█") (alnum | "-" | ":" | "█")*
   trailingTagNameTextNode := (alnum | "-" | ":" | "█")+
 }

packages/liquid-html-parser/src/stage-1-cst.spec.ts

@@ -1383,6 +1383,16 @@ describe('Unit: Stage 1 (CST)', () => {
       expectPath(cst, '0.markup.filters.0.args.1.value.type').to.eql('VariableLookup');
       expectPath(cst, '0.markup.filters.0.args.1.value.name').to.eql('█');
     });
+
+    it('should parse incomplete parameters for filters', () => {
+      const toCST = (source: string) => toLiquidHtmlCST(source, { mode: 'completion' });
+
+      cst = toCST(`{{ a[1].foo | image_url: 200, width: 100, h█ }}`);
+
+      expectPath(cst, '0.markup.filters.0.args.0.type').to.equal('Number');
+      expectPath(cst, '0.markup.filters.0.args.1.type').to.equal('NamedArgument');
+      expectPath(cst, '0.markup.filters.0.args.2.type').to.equal('VariableLookup');
+    });
   });
 
   function makeExpectPath(message: string) {

packages/liquid-html-parser/src/stage-1-cst.ts

@@ -860,7 +860,18 @@ function toCST<T>(
         }
       },
     },
+    filterArguments: 0,
     arguments: 0,
+    complexArguments: function (completeParams, _space1, _comma, _space2, incompleteParam) {
+      const self = this as any;
+
+      return completeParams
+        .toAST(self.args.mapping)
+        .concat(
+          incompleteParam.sourceString === '' ? [] : incompleteParam.toAST(self.args.mapping),
+        );
+    },
+    simpleArgument: 0,
     tagArguments: 0,
     positionalArgument: 0,
     namedArgument: {

packages/theme-check-common/src/types/theme-liquid-docs.ts

@@ -131,6 +131,7 @@ export interface Parent {
 export interface Parameter {
   description: string;
   name: string;
+  positional: boolean;
   required: boolean;
   types: string[];
 }

packages/theme-language-server-common/src/completions/CompletionsProvider.ts

@@ -17,6 +17,7 @@ import {
   Provider,
   RenderSnippetCompletionProvider,
   TranslationCompletionProvider,
+  FilterNamedParameterCompletionProvider,
 } from './providers';
 import { GetSnippetNamesForURI } from './providers/RenderSnippetCompletionProvider';
 
@@ -65,6 +66,7 @@ export class CompletionsProvider {
       new FilterCompletionProvider(typeSystem),
       new TranslationCompletionProvider(documentManager, getTranslationsForURI),
       new RenderSnippetCompletionProvider(getSnippetNamesForURI),
+      new FilterNamedParameterCompletionProvider(themeDocset),
     ];
   }
 

packages/theme-language-server-common/src/completions/providers/FilterCompletionProvider.spec.ts

@@ -43,6 +43,41 @@ const filters: FilterEntry[] = [
     name: 'default',
     syntax: 'variable | default: variable',
     return_type: [{ type: 'untyped', name: '' }],
+    parameters: [
+      {
+        description: 'Whether to use false values instead of the default.',
+        name: 'allow_false',
+        positional: true,
+        required: false,
+        types: ['boolean'],
+      },
+    ],
+  },
+  {
+    syntax: 'string | highlight: string',
+    name: 'highlight',
+    parameters: [
+      {
+        description: 'The string that you want to highlight.',
+        name: 'highlighted_term',
+        positional: true,
+        required: true,
+        types: ['string'],
+      },
+    ],
+  },
+  {
+    syntax: 'string | preload_tag: as: string',
+    name: 'preload_tag',
+    parameters: [
+      {
+        description: 'The type of element or resource to preload.',
+        name: 'as',
+        positional: false,
+        required: true,
+        types: ['string'],
+      },
+    ],
   },
   {
     name: 'missing_syntax',
@@ -133,6 +168,161 @@ describe('Module: FilterCompletionProvider', async () => {
     // As in, the anyFilters are at the _end_ and not shown at the top.
     await expect(provider).to.complete('{{ string | █ }}', stringFilters.concat(anyFilters));
   });
+
+  describe('when there are no required parameters', () => {
+    it('should not include parameters in the insertText of the completion', async () => {
+      //                  char 12 ⌄   ⌄ char 16
+      const liquid = '{{ string | defa█ }}';
+
+      await expect(provider).to.complete(liquid, [
+        expect.objectContaining({
+          label: 'default',
+          insertTextFormat: 1,
+          textEdit: expect.objectContaining({
+            newText: 'default',
+            range: {
+              end: {
+                line: 0,
+                character: 16,
+              },
+              start: {
+                line: 0,
+                character: 12,
+              },
+            },
+          }),
+        }),
+      ]);
+    });
+  });
+
+  describe('when there are required positional parameters', () => {
+    it('should include parameters in the insertText of the completion', async () => {
+      //                  char 12 ⌄  ⌄ char 15
+      const liquid = '{{ string | hig█ }}';
+
+      await expect(provider).to.complete(liquid, [
+        expect.objectContaining({
+          label: 'highlight',
+          insertTextFormat: 2,
+          textEdit: expect.objectContaining({
+            newText: "highlight: '${1:highlighted_term}'",
+            range: {
+              end: {
+                line: 0,
+                character: 15,
+              },
+              start: {
+                line: 0,
+                character: 12,
+              },
+            },
+          }),
+        }),
+      ]);
+    });
+  });
+
+  describe('when there are required named parameters', () => {
+    it('should include parameters in the insertText of the completion', async () => {
+      //                  char 12 ⌄  ⌄ char 15
+      const liquid = '{{ string | pre█ }}';
+
+      await expect(provider).to.complete(liquid, [
+        expect.objectContaining({
+          label: 'preload_tag',
+          insertTextFormat: 2,
+          textEdit: expect.objectContaining({
+            newText: "preload_tag: as: '$1'",
+            range: {
+              end: {
+                line: 0,
+                character: 15,
+              },
+              start: {
+                line: 0,
+                character: 12,
+              },
+            },
+          }),
+        }),
+      ]);
+    });
+  });
+
+  describe('when the cursor is in the middle of the filter', () => {
+    describe('and the filter can only be completed to the same name', () => {
+      it('sets the range to only the existing name', async () => {
+        //                  char 12 ⌄          ⌄ char 23
+        const liquid = '{{ string | prel█oad_tag: as: "p" }}';
+
+        await expect(provider).to.complete(liquid, [
+          expect.objectContaining({
+            label: 'preload_tag',
+            insertTextFormat: 1,
+            textEdit: expect.objectContaining({
+              newText: 'preload_tag',
+              range: {
+                end: {
+                  line: 0,
+                  character: 23,
+                },
+                start: {
+                  line: 0,
+                  character: 12,
+                },
+              },
+            }),
+          }),
+        ]);
+      });
+    });
+
+    describe('and the filter to be replaced has parameters', () => {
+      it('sets the range to include the parameters if replacing with a different filter', async () => {
+        //                  char 12 ⌄            ⌄ char 25
+        const liquid = '{{ string | d█efault: true }}';
+        //                                 ⌃ char 19
+
+        await expect(provider).to.complete(liquid, [
+          expect.objectContaining({
+            label: 'downcase',
+            insertTextFormat: 1,
+            textEdit: expect.objectContaining({
+              newText: 'downcase',
+              range: {
+                end: {
+                  line: 0,
+                  character: 25,
+                },
+                start: {
+                  line: 0,
+                  character: 12,
+                },
+              },
+            }),
+          }),
+          expect.objectContaining({
+            label: 'default',
+            insertTextFormat: 1,
+            textEdit: expect.objectContaining({
+              newText: 'default',
+              range: {
+                end: {
+                  line: 0,
+                  character: 19,
+                },
+                start: {
+                  line: 0,
+                  character: 12,
+                },
+              },
+            }),
+          }),
+        ]);
+      });
+    });
+  });
 });
 
 function filtersNamesOfInputType(inputType: string): string[] {