Pass snippets as argument to engine

[AndreasArvidsson]

Makes it so that Cursorless everywhere editors doesn't have to provide a snippet implementation

Checklist

• [/] I have added tests
• [/] I have updated the docs and cheatsheet
• [/] I have not broken the cheatsheet

[pokey]

I haven't read in detail, but my first two reactions are as follows:

Moving snippets from engine to vscode feels like a lateral at best. We went through the effort to abstract vscode out of the snippets component, so I would prefer if we didn't reverse that. Ie this makes it harder for us to use snippets in neovim or other node contexts

Second issue is that it doesn't really make the core much leaner if we still have a bunch of references to snippets in the engine. They're only used in two actions right? I'd be tempted to think the right abstraction is to allow passing in custom actions. Then snippets are never even mentioned

But I didn't read super closely so maybe missing something

[AndreasArvidsson]

I was thinking that since snippets is probably going to be deprecated this was a reasonable decision. When we make the next version we can do it more properly. But right now I'm blocked on this and I don't think any other editor is going to want to implement our json based snippets in the coming few weeks?

This is just about getting rid of node references in engine. A much better interface for snippets could definitely be designed.

With that said let me have another look at it tomorrow and I might come up with a solution you like better :)

[AndreasArvidsson]

I've had another look and started a more more abstracted implementation and bailed. Basically most of what is in Snippets.ts that I here reamed to VscodeSnippets.ts is vscode specific. The fact that we are polling the file system. The handling of directory error message. The fact that you can a register third party snippets to some extent. If I remove everything that isn't vscode and make an abstraction it basically becomes more code for no good reason. I stand by my original assessment that this should probably live in vscode until we make a refactor where we used the new snippet format and don't poll.

[AndreasArvidsson]

Implementations like this also makes it a bit hard to abstract snippets. We are explicitly creating a file in the file system and then opening in the editor. To open the file we need a filepath so we are still stuck in the file system with snippets. Here my inclination is just to drill the file system instance through and add touch to that. We can later on revamp the entire snippet system and make its smarter, but for now I can a just need to be unblocked. Sounds reasonable?

cursorless/packages/cursorless-engine/src/actions/GenerateSnippet/openNewSnippetFile.ts

Line 20 in 2e69df8

await touch(path);

[pokey]

Implementations like this also makes it a bit hard to abstract snippets. We are explicitly creating a file in the file system and then opening in the editor. To open the file we need a filepath so we are still stuck in the file system with snippets. Here my inclination is just to drill the file system instance through and add touch to that. We can later on revamp the entire snippet system and make its smarter, but for now I can a just need to be unblocked. Sounds reasonable?

cursorless/packages/cursorless-engine/src/actions/GenerateSnippet/openNewSnippetFile.ts

Line 20 in 2e69df8

await touch(path);

Wouldn't my second suggestion above solve this problem? Ie generate snippet, insert snippet and wrap with snippet are just custom actions passed into the engine. If this isn't making sense let's discuss tomorrow alongside vscode vs engine thing

[AndreasArvidsson]

That sounds like a reasonable solution, but to be fair I don't really have the energy for that right now. This was only moving a single file that will unblock me. And we can then later make a more flexible approach. Happy to discuss it tomorrow.

[pokey]

Let's discuss tomorrow

[pokey]

update from meet-up: pokey caved

[pokey]

The one thing I think we discussed changing here is creating a DisabledSnippets class so that we can confine null checks to createEngine. Otherwise looks good; see also #2353 (comment)

[AndreasArvidsson]

Done

[pokey]

Looks good with minor nit