-
-
Notifications
You must be signed in to change notification settings - Fork 36
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
csv overrides #63
csv overrides #63
Conversation
Fwiw I get the following error when checking out this branch:
But everything works great, and when I reload talon I don't get the error on startup. Seems fine to me |
There probably she reason why relative imports is discouraged |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great with a bunch of minor comments. Also feel free to merge #64 into your branch if it looks good. As I was hacking, I noticed that cursorless was pretty much bricked if there was an error on one line of a csv, which is kind of against the talon philosophy of "yell, but plough on if you can"
Also, a couple other thoughts:
- It might be worth having a special spoken form value that users can use to disable an action / scope type they don't want. Eg
<DISABLED>, setSelection
- What happens if we ever remove an action / scope type? I guess we prob shouldn't do that as it breaks backwards compatibility, but I could see us merging scope types / actions or something
Co-authored-by: Pokey Rule <pokey.rule@gmail.com>
Co-authored-by: Pokey Rule <pokey.rule@gmail.com>
Co-authored-by: Pokey Rule <pokey.rule@gmail.com>
Co-authored-by: Pokey Rule <pokey.rule@gmail.com>
I don't think we need a voice command to disable a command. Is better to just edit the override file. Removing an identifier would be handled the same way as renaming. ie with an update list |
exactly, but I think
yep makes sense. let's try to avoid it but agreed that's prob the right way to handle it if / when it happens |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
Do we want to handle inside outside before merging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait one sec. going to disable shapes for this release to give us a couple days to tweak
To do:
connective.py
and use it for bring / move / swapWon't do: