Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added voice command to migrate Cursorless snippet to community format #2747

Merged
merged 35 commits into from
Jan 28, 2025

Conversation

AndreasArvidsson
Copy link
Member

@AndreasArvidsson AndreasArvidsson commented Jan 23, 2025

2 fields in the Cursorless snippets are not available in the community format

  1. scopeTypes. To my knowledge the only use case is omitting the function keyword when in a javascript class
  2. excludeDescendantScopeTypes: Used in conjunction with scopeTypes to include the function keyword when in a function inside a class.

scopeTypes and excludeDescendantScopeTypes I'm not to sure we want to add to community. First of all they have a single use case and needing two separate fields just to describe this behavior fields a bit verbose.
As well community doesn't have support for sending multiple snippets.

I'm planning this as a follow up.

Reference

export interface SnippetScope {
/**
* VSCode language ids where this snippet definition should be active
*/
langIds?: string[];
/**
* Cursorless scopes in which this snippet is active. Allows, for example, to
* have different snippets to define a function if you're in a class or at
* global scope.
*/
scopeTypes?: SimpleScopeTypeType[];
/**
* Exclude regions of {@link scopeTypes} that are descendants of these scope
* types. For example, if you have a snippet that should be active in a class
* but not in a function within the class, you can specify
* `scopeTypes: ["class"], excludeDescendantScopeTypes: ["namedFunction"]`.
*/
excludeDescendantScopeTypes?: SimpleScopeTypeType[];
}

Fixes #2355

Checklist

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

@AndreasArvidsson AndreasArvidsson requested a review from a team as a code owner January 23, 2025 18:57
}

async function migrateFile(targetDirectory: string, filePath: string) {
const fileName = path.basename(filePath, ".cursorless-snippets");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CURSORLESS_SNIPPETS_SUFFIX?


async function writeCommunityFile(snippetFile: SnippetFile, filePath: string) {
const snippetText = serializeSnippetFile(snippetFile);
const file = await fs.open(filePath, "w");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking for file existence separately from opening the file is generally racy, best to just do this:

Suggested change
const file = await fs.open(filePath, "w");
const file = await fs.open(filePath, "wx");

'wx': Like 'w' but fails if the path exists.

Then catch and put your conflict filename behavior in the catch block.

@phillco phillco enabled auto-merge January 28, 2025 19:16
@phillco phillco added this pull request to the merge queue Jan 28, 2025
Merged via the queue into main with commit c690d93 Jan 28, 2025
15 checks passed
@phillco phillco deleted the migrateSnippets branch January 28, 2025 19:34
cursorless-bot pushed a commit that referenced this pull request Jan 28, 2025
…#2747)

2 fields in the Cursorless snippets are not available in the community
format

1. scopeTypes. To my knowledge the only use case is omitting the
`function` keyword when in a javascript class
2. excludeDescendantScopeTypes: Used in conjunction with `scopeTypes` to
include the function keyword when in a function inside a class.

scopeTypes and excludeDescendantScopeTypes I'm not to sure we want to
add to community. First of all they have a single use case and needing
two separate fields just to describe this behavior fields a bit verbose.
As well community doesn't have support for sending multiple snippets. 

I'm planning this as a follow up. 

Reference

https://github.com/cursorless-dev/cursorless/blob/57acd6c0fcc32e5df122ce09bb26bde5f09495ca/packages/common/src/types/snippet.types.ts#L4-L24

Fixes #2355

## Checklist

- [ ] I have added
[tests](https://www.cursorless.org/docs/contributing/test-case-recorder/)
- [x] I have updated the
[docs](https://github.com/cursorless-dev/cursorless/tree/main/docs) and
[cheatsheet](https://github.com/cursorless-dev/cursorless/tree/main/cursorless-talon/src/cheatsheet)
- [/] I have not broken the cheatsheet

---------

Co-authored-by: pre-commit-ci-lite[bot] <117423508+pre-commit-ci-lite[bot]@users.noreply.github.com>
Co-authored-by: Phil Cohen <phillip@phillip.io>
@pokey
Copy link
Member

pokey commented Jan 28, 2025

What does this tool do with the scopeTypes and excludeDescendantScopeTypes fields if you're using them?

@AndreasArvidsson
Copy link
Member Author

What does this tool do with the scopeTypes and excludeDescendantScopeTypes fields if you're using them?

Discards them. These fields are not really used that much (There is only one use case as far as I know). And since we haven't decided on the format we are going to use in the future I want to avoid a community to community migration script if we decide to use something else.

@pokey
Copy link
Member

pokey commented Jan 28, 2025

I would bail if you encounter them. Silently dropping data is not good ux

@AndreasArvidsson
Copy link
Member Author

You mean bail on the entire migration or just on that snippet?

@pokey
Copy link
Member

pokey commented Jan 28, 2025

Maybe just that snippet? I don't feel strongly, as long as we raise a flag rather than silently losing data

@AndreasArvidsson
Copy link
Member Author

Totally agree. Here is the follow up.
#2789

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create "migrate snippets" command that will automatically migrate all of a user's snippets
3 participants