Conversation
pokey
left a comment
pokey
left a comment
There was a problem hiding this comment.
I reviewed until I realised this was a modifier, and then stopped, because that's a pretty fatal flaw imo
packages/cursorless-engine/src/processTargets/modifiers/GlyphStage.ts
Outdated
Show resolved
Hide resolved
packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/RegexScopeHandler.ts
Outdated
Show resolved
Hide resolved
pokey
left a comment
pokey
left a comment
There was a problem hiding this comment.
Added spoken form generation in https://github.com/cursorless-dev/cursorless/pull/2050/files/f71f10bb58899de588c834b02effd5005a2d0987..a9473540a5e42be455a9d2d349b22b47b7ac1d59.
I still need to add tests for the custom spoken form generation. That will take a little effort, as we only have good infra for testing custom spoken forms for scopes yielded by our scope provider, which doesn't yield glyph scopes. I'll take a crack at that tomorrow. If it gets too painful I'll put it in a separate PR
|
Not sure glyph and literal are the same. Arguably glyph should go through same grapheme normalisation as hat chars, whereas literal wouldn't necessarily |
|
Then do you argue that I should revert this last commit back to only matching a single character? Because the implementation python side of this is the same capture that we use for decorated marks. |
|
I'm just arguing maybe we keep the same naming convention you had before (glyph scope / character as field). I don't feel strongly, just a thought. Maybe let's discuss if you disagree / aren't sure? |
|
I'm happy with reverting |
This reverts commit 4ed60fb.
|
Reverted to the old naming scheme |
pokey
left a comment
pokey
left a comment
There was a problem hiding this comment.
Ok I added a test for custom spoken form generation. I left one more comment. Once that's addressed, and if you're happy with my changes, let's ship it!
There was a problem hiding this comment.
Cheatsheet seems to still have it as a modifier
| spokenForms: customSpokenForms, | ||
| requiresTalonUpdate: false, | ||
| isCustom: isEqual(defaultSpokenForms, customSpokenForms), | ||
| isCustom: !isEqual(defaultSpokenForms, customSpokenForms), |
There was a problem hiding this comment.
whoops 😅. we don't have any use case for this yet, but should prob add a test at some point
There was a problem hiding this comment.
Really proves that if something isn't tested it can easily be incorrect :)
`chuck glyph dollar red air` Fixes #54 ## Checklist - [x] 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] I have not broken the cheatsheet --------- Co-authored-by: Pokey Rule <755842+pokey@users.noreply.github.com>
`chuck glyph dollar red air` Fixes cursorless-dev#54 ## Checklist - [x] 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] I have not broken the cheatsheet --------- Co-authored-by: Pokey Rule <755842+pokey@users.noreply.github.com>
3 participants
chuck glyph dollar red airFixes #54
Checklist