Skip to content

Conversation

AndreasArvidsson
Copy link
Member

@AndreasArvidsson AndreasArvidsson commented Feb 10, 2025

Read from deprecated snippet csv files if the exists, but do not create them if they don't.

Fixes #2841

@AndreasArvidsson AndreasArvidsson requested a review from a team as a code owner February 10, 2025 08:19
@AndreasArvidsson AndreasArvidsson marked this pull request as draft February 10, 2025 08:25
@AndreasArvidsson AndreasArvidsson marked this pull request as ready for review February 11, 2025 11:09
)

fs.watch(str(file_path.parent), on_watch)
fs.watch(file_path.parent, on_watch)
Copy link
Member Author

Choose a reason for hiding this comment

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

Type change in Talon. You can pass either a path instance or a string and both will work, but only a path object does not give type errors.

Copy link
Member

@pokey pokey left a comment

Choose a reason for hiding this comment

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

looks good with minor comment

"-from": "experimental.setInstanceReference"
}
},
"experimental/wrapper_snippets.csv": {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we want to be more explicit. Just allowing us to leave out things feels like it's making it easier to make mistakes. Maybe we use a string "DEPRECATED"?

Copy link
Member Author

@AndreasArvidsson AndreasArvidsson Feb 11, 2025

Choose a reason for hiding this comment

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

I'm not sure what keeping the deprecated spoken forms in the json file helps with?
We still need to specify the file name otherwise we don't know which csv file to read if it exists. So we will never be matching anything against this files if we remove the path in the key. Or did you mean that "DEPRECATED" should be the value? I could also add an extra argument to init_csv_and_watch_changes instead of passing none as the default values?

Another option is to just set null as the value in the json file. Then we don't need to match against a particular string value.

A boolean argument to init_csv_and_watch_changes is probably my favorite solution of these.

Copy link
Member Author

Choose a reason for hiding this comment

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

I went ahead with a boolean solution

@AndreasArvidsson AndreasArvidsson added this pull request to the merge queue Feb 14, 2025
Merged via the queue into main with commit 9fa646b Feb 14, 2025
16 checks passed
@AndreasArvidsson AndreasArvidsson deleted the snippetSpokenForms branch February 14, 2025 10:00
cursorless-bot pushed a commit that referenced this pull request Feb 14, 2025
Read from deprecated snippet csv files if the exists, but do not create
them if they don't.

Fixes #2841
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.

Remove snippet csv support

2 participants