Skip to content

Commit

Permalink
Use textEdit instead of insertText for filter completions
Browse files Browse the repository at this point in the history
This fixes a bug where if you try to complete a filter while in the
middle of an existing filter's text we'd just append the completion. Now
we'll replace the filter you're running the completion on.
  • Loading branch information
graygilmore committed Nov 28, 2024
1 parent 6f1318e commit 4b2dec3
Show file tree
Hide file tree
Showing 3 changed files with 201 additions and 11 deletions.
5 changes: 5 additions & 0 deletions .changeset/metal-seahorses-breathe.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@shopify/theme-language-server-common': patch
---

Fix a bug where filter completions wouldn't replace existing text
Original file line number Diff line number Diff line change
Expand Up @@ -171,39 +171,158 @@ describe('Module: FilterCompletionProvider', async () => {

describe('when there are no required parameters', () => {
it('should not include parameters in the insertText of the completion', async () => {
await expect(provider).to.complete('{{ string | defa█ }}', [
// char 12 ⌄ ⌄ char 16
const liquid = '{{ string | defa█ }}';

await expect(provider).to.complete(liquid, [
expect.objectContaining({
label: 'default',
insertText: 'default',
insertTextFormat: 1,
textEdit: expect.objectContaining({
newText: 'default',
range: {
end: {
line: 0,
character: 16,
},
start: {
line: 0,
character: 12,
},

This comment has been minimized.

Copy link
@charlespwd

charlespwd Nov 28, 2024

Contributor

Shoot forgot to tell you there's an easier way to test these assertions.

IIRC the TextDocument class has a static applyEdit method that lets you write assertions like

expect(TextDocument.applyEdit(docBeforeChange, textEdit)).to.equal(stringAfterChange)
},
}),
}),
]);
});
});

describe('when there are required positional parameters', () => {
it('should include parameters in the insertText of the completion', async () => {
await expect(provider).to.complete('{{ string | h█ }}', [
// char 12 ⌄ ⌄ char 15
const liquid = '{{ string | hig█ }}';

await expect(provider).to.complete(liquid, [
expect.objectContaining({
label: 'highlight',
insertText: "highlight: '${1:highlighted_term}'",
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 () => {
await expect(provider).to.complete('{{ string | pre█ }}', [
// char 12 ⌄ ⌄ char 15
const liquid = '{{ string | pre█ }}';

await expect(provider).to.complete(liquid, [
expect.objectContaining({
label: 'preload_tag',
insertText: "preload_tag: as: '$1'",
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[] {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,14 @@
import { NodeTypes } from '@shopify/liquid-html-parser';
import { LiquidFilter, NodeTypes } from '@shopify/liquid-html-parser';
import { FilterEntry, Parameter } from '@shopify/theme-check-common';
import { CompletionItem, CompletionItemKind, InsertTextFormat } from 'vscode-languageserver';
import {
CompletionItem,
CompletionItemKind,
InsertTextFormat,
TextEdit,
} from 'vscode-languageserver';
import { PseudoType, TypeSystem, isArrayType } from '../../TypeSystem';
import { memoize } from '../../utils';
import { AugmentedLiquidSourceCode } from '../../documents';
import { CURSOR, LiquidCompletionParams } from '../params';
import { Provider, createCompletionItem, sortByName } from './common';

Expand Down Expand Up @@ -46,20 +52,80 @@ export class FilterCompletionProvider implements Provider {
return options
.filter(({ name }) => name.startsWith(partial))
.map((entry) => {
const { insertText, insertStyle } = appendRequiredParemeters(entry);
const { textEdit, format } = this.textEdit(node, params.document, entry);

return createCompletionItem(
entry,
{
kind: CompletionItemKind.Function,
insertText,
insertTextFormat: insertStyle,
insertTextFormat: format,
textEdit,
},
'filter',
);
});
}

textEdit(
node: LiquidFilter,
document: AugmentedLiquidSourceCode,
entry: MaybeDeprioritisedFilterEntry,
): {
textEdit: TextEdit;
format: InsertTextFormat;
} {
const remainingText = document.source.slice(node.position.end);

// Match all the way up to the termination of the filter which could be
// another filter (`|`), or the end of a liquid statement.
const matchEndOfFilter = remainingText.match(/^(.*?)\s*(?=\||-?\}\}|-?\%\})|^(.*)$/);
const endOffset = matchEndOfFilter ? matchEndOfFilter[1].length : remainingText.length;

// The start position for a LiquidFilter node includes the `|`. We need to
// ignore the pipe and any spaces for our starting position.
const pipeRegex = new RegExp(`(\\s*\\|\\s*)(?:${node.name}\\}\\})`);
const matchFilterPipe = node.source.match(pipeRegex);
const startOffet = matchFilterPipe ? matchFilterPipe[1].length : 0;

let start = document.textDocument.positionAt(node.position.start + startOffet);
let end = document.textDocument.positionAt(node.position.end + endOffset);

const { insertText, insertStyle } = appendRequiredParemeters(entry);

let newText = insertText;
let format = insertStyle;

// If the cursor is inside the filter or at the end and it's the same
// value as the one we're offering a completion for then we want to restrict
// the insert to just the name of the filter.
// e.g. `{{ product | imag█e_url: crop: 'center' }}` and we're offering `imag█e_url`
const existingFilterOffset = remainingText.match(/[^a-zA-Z_]/)?.index ?? remainingText.length;
if (node.name + remainingText.slice(0, existingFilterOffset) === entry.name) {
newText = entry.name;
format = InsertTextFormat.PlainText;
end = document.textDocument.positionAt(node.position.end + existingFilterOffset);
}

// If the cursor is at the beginning of the string we can consider all
// options and should not replace any text.
// e.g. `{{ product | █image_url: crop: 'center' }}`
// e.g. `{{ product | █ }}`
if (node.name === '█') {
end = start;
}

return {
textEdit: TextEdit.replace(
{
start,
end,
},
newText,
),
format,
};
}

options: (inputType: PseudoType) => Promise<MaybeDeprioritisedFilterEntry[]> = memoize(
async (inputType) => {
const filterEntries = await this.typeSystem.filterEntries();
Expand Down

0 comments on commit 4b2dec3

Please sign in to comment.