Skip to content

Conversation

AndreasArvidsson
Copy link
Member

@AndreasArvidsson AndreasArvidsson commented Aug 1, 2023

Depends on #1637

Checklist

  • [/] I have added tests
  • I have updated the docs and cheatsheet
  • Update cheatsheet spoken form defaults
  • Manually test the various snippet terms
  • Manually test the snippet apis
  • I have not broken the cheatsheet

@AndreasArvidsson AndreasArvidsson marked this pull request as ready for review August 2, 2023 07:51
@AndreasArvidsson AndreasArvidsson requested a review from pokey as a code owner August 2, 2023 07:51
@pokey pokey force-pushed the destination-talon branch from 793da45 to 692575b Compare August 2, 2023 18:07
@AndreasArvidsson
Copy link
Member Author

@pokey All tests are passing at my end. I checked and the ctx argument is not used and can be removed.

ctx: Context = Context(),

@AndreasArvidsson AndreasArvidsson added the to discuss Plan to discuss at meet-up label Aug 3, 2023
@pokey
Copy link
Member

pokey commented Aug 3, 2023

Yep they were working fine for me too last I checked 👍.

I just realised why we have that unused context arg: we were using it for experimental snippet stuff and then removed that tag. I guess we might use it for experimental stuff in the future, but I'm also fine with just removing it for now 🤷‍♂️

@AndreasArvidsson
Copy link
Member Author

I think we can remove it for now and it's easy enough to readd if we need it in the future.

@AndreasArvidsson AndreasArvidsson removed the to discuss Plan to discuss at meet-up label Aug 3, 2023
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.

Good stuff! Made a bunch of minor tweaks, and left comments for the ones I wasn't sure about. I'm happy to make the changes or let you do it; either way

@pokey
Copy link
Member

pokey commented Aug 3, 2023

Re removing ctx arg, maybe let's leave that for the follow-up where I unify the on_ready callbacks

@AndreasArvidsson
Copy link
Member Author

Re removing ctx arg, maybe let's leave that for the follow-up where I unify the on_ready callbacks

Sounds good

@AndreasArvidsson
Copy link
Member Author

@pokey Updated with requested changes

if hasattr(m, "cursorless_insertion_mode_to"):
words = m._unmapped
actions.app.notify(
f"'{' '.join(words)}' is deprecated. Please just say '{words[-1]}'"
Copy link
Member

Choose a reason for hiding this comment

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

Not sure whether it's worth fixing, but this doesn't correctly handle the case where they have more than one word in their spoken form for "before" or "after", right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly. This will just utilize the words they used for this phrase. Which I think is probably good enough.

Copy link
Member

@pokey pokey Aug 4, 2023

Choose a reason for hiding this comment

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

Right but eg if they have "prior to" as their spoken form for before, then if they say "to prior to", it will say

'to prior to' is deprecated. Please just say 'to'

correct? I guess we can live with that; I have to imagine remapping "before" to a two-word modifier is exceedingly rare, if anyone has done it at all

Copy link
Member Author

Choose a reason for hiding this comment

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

You are probably right. Should be very rare

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.

Good stuff! I think it's time to ship it

@pokey pokey added this pull request to the merge queue Aug 6, 2023
Merged via the queue into main with commit 41e0c26 Aug 6, 2023
@pokey pokey deleted the destination-talon branch August 6, 2023 15:21
cursorless-bot pushed a commit that referenced this pull request Aug 6, 2023
Depends on #1637

- Fixes #725
- Partially addresses #1440
- Fixes #1319 

## 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)
- [x] Update cheatsheet spoken form defaults
- [x] Manually test the various snippet terms
- [x] Manually test the snippet apis
- [ ] I have not broken the cheatsheet

---------

Co-authored-by: Pokey Rule <[email protected]>
Co-authored-by: pre-commit-ci-lite[bot] <117423508+pre-commit-ci-lite[bot]@users.noreply.github.com>
thetomcraig-aya pushed a commit to thetomcraig/cursorless that referenced this pull request Mar 27, 2024
Depends on cursorless-dev#1637

- Fixes cursorless-dev#725
- Partially addresses cursorless-dev#1440
- Fixes cursorless-dev#1319 

## 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)
- [x] Update cheatsheet spoken form defaults
- [x] Manually test the various snippet terms
- [x] Manually test the snippet apis
- [ ] I have not broken the cheatsheet

---------

Co-authored-by: Pokey Rule <[email protected]>
Co-authored-by: pre-commit-ci-lite[bot] <117423508+pre-commit-ci-lite[bot]@users.noreply.github.com>
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.

Fix 'paste' documentation Use data classes on talon side in order to get better type hints
2 participants