Community wrapper snippets

[AndreasArvidsson]

This is the Cursorless side of the community wrapper snippets

talonhub/community#1315

Checklist

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

[pokey]

This looks fine but what happens if they're on old version of community? Need to make sure we degrade gracefully

Also, I believe there's now a spoken form ambiguity with "funk wrap", no?

I wonder if it would be better for talon to just tell Cursorless where the snippets are using the new state.json file and let it parse them itself so it can handle disambiguating 🤔

[AndreasArvidsson]

I think it reasonable to expect people to update community before cursorless. I know of no way of using community lists or captures in cursorless that would not show an error in the log if missing.

The ambiguity would be there if we added the command to community as well. You just cant define the same snippet phrase in both places.

I prefer the rpc solution.

[pokey]

I think it reasonable to expect people to update community before cursorless. I know of no way of using community lists or captures in cursorless that would not show an error in the log if missing.

I completely disagree. We have gone out of our way to make Cursorless talon files easy to update by supporting extensive customization. It is not uncommon for people to go months without updating community, because the fact that the only way to tweak community oftentimes is to directly change it means that upgrades can be quite painful

We should discuss a way to make Cursorless backwards compatible with community. We've done it before, but I believe that was for a new action, so we could easily wrap it in a try-catch

The ambiguity would be there if we added the command to community as well. You just cant define the same snippet phrase in both places.

Not saying it wouldn't, just pointing it out. Could be surprising if you change your function snippet in community and it doesn't update because Cursorless is preferring the Cursorless one

[AndreasArvidsson]

Doesn't that mean that we can never utilize new list or captures from community? I know of no mechanism to solve that.

Yes you can't have the same snippet in both. Personally I have totally disabled cursorless snippets. Getting around this problem might be interesting.

[pokey]

Doesn't that mean that we can never rely on new list or captures from community? I know of no mechanism to solve that.

We could add a context key that we set from community and then match on that?

Yes you cant have the same snippet in both. Personally I have totally disabled cursorless snippets. Getting around this problem might be interesting.

Yeah would be worth discussing. Esp since we ship with a conflicting snippet 🤔 to discuss Plan to discuss at meet-up

[AndreasArvidsson]

We could add a context key that we set from community and then match on that?

Unfortunately results in the same problem. If that tag is missing from community because of an older version then cursorless would error in the log.

Yeah would be worth discussing. Esp since we ship with a conflicting snippet 🤔

Agreed.

[pokey]

No not a tag a context key. Do those have to be declared?

[AndreasArvidsson]

No not a tag a context key. Do those have to be declared?

Not sure I follow. Do you mean like @mod.scope?

Probably same problem

[pokey]

Not sure. You don't explicitly declare the key for a scope in the same way as you declare a tag, so it seems possible that it won't throw an exception if undefined.

If that doesn't work, we could possibly make an onready in cursorless talon that tries to call some action (wrapped in a try-catch) to see whether it exists, and if it does, sets a tag

Anyways we can discuss if none of these options make sense to you; there's usually a way to make things work, with enough elbow grease 🙂

[pokey]

update from meet-up:

• asked aegis:

We introduced a new list in community, and would like to rely on the list in cursorless-talon. However, we expect users to upgrade cursorless-talon more frequently than community, and thus can’t assume that they have this list already. How can we leverage this list when it’s present, without things breaking when it isn’t?

• @pokey will review Added insertions scope for insert snippet action #1879
• introduce user.cursorless_use_community_snippets tag that will enable wrapping / destinational insertion and disable cursorless snippet wrapping / insertion

[pokey]

I wonder if it's worth considering sharing the snippets from Talon to Cursorless via .cursorless/state.json. Then we could eg show them in the VSCode Cursorless side bar, handle scope-specific snippets, use them for fancy Cursorless stuff like #446 etc

[AndreasArvidsson]

@pokey updated

[pokey]

left a couple comments

[pokey]

made minor fix; otherwise looks good! See comments / fix in talonhub/community#1315 as well

[AndreasArvidsson]

Thanks. Community updated with your suggestion

[pokey]

Ok looking good. Is there a way to test this? Ie could we activate the use_community tag for part of our tests?

[AndreasArvidsson]

Probably doable?

[pokey]

update from meet-up:

• we do want tests
• @AndreasArvidsson will keep this for now, but if time doesn't allow, @pokey will steal this PR in a week

[pokey]

There is a spoken form inconsistency here: in community, the spoken form for snippet insertion is "snip", but the default spoken form for snippet insertion in Cursorless is "snippet". So out of the box, it will be "snip funk", but "snippet funk after air". Not entirely sure how to handle that 🤔

[pokey]

Ok I added tests, but:

• See caveat in https://github.com/talonhub/community/pull/1315/files#r1471234714
• It relies on user having "code" snippet (code block). That's shipped in community, so should be fine, but we should really mock community snippet dir setting to make this more robust. Prob fine for now tho I think as long as we add a caveat somewhere

[pokey]

update from meet-up:

• run one last local test and then ship this alongside Get snippets actions talonhub/community#1315

[brief]

Is this not merging because of the failing test?

[pokey]

Yeah it got caught in that weird windows CI issue; totally unrelated to this PR. Just updated the branch so it gets the fix. Should pass and merge shortly