move definition of which actions exit cursorless mode to the config #2174
Conversation
Unfortunately, package.json is not as expressive as one might hope. There doesn't appear to be a way to allow objects and yet also restrict any strings that appear to a list of enums. Fixes #2117
Works fine for me: 755806c. Did that not work for you? edit: I see, the autocomplete is flawless, but you get an annoying yellow squiggly underline when you use the object. That's annoying |
|
I would be tempted to live with the yellow squiggly rather than lose autocomplete for the common case of simple id |
|
ok figured it out. See latest push |
Nice! The joys of DSLs. All the changes you added LGTM, thank you! (I still don't really know how to write nice idiomatic TypeScript.) I definitely appreciate the "I'll just fix this trivial stuff for you instead of playing tennis". (And the non-trivial stuff!) I should really do more of that myself. |
|
Note that there's also a lodash isString, which we use elsewhere in the code base. Wasn't sure whether that was intentional or not. |
yeah I think that model works well in open source because people appreciate the saved time. in a company I don't think it works as well because it can become a turf war 😅 |
i think maybe it has to do with the personalities involved instead? or both? either way, happy trails right here right now, which is all one can really ask for in life. |
|
ok i made a flailing attempt to clean things up. i haven't looked to closely at the result but typescript stopped complaining after shouting at me for hours so I'm declaring victory; may need some cleanup / fixes if tests are broken but I think you should be able to pick it back up from here |
|
OK. Happy to pick it back up and get stuff working again, but I have to admit I don't understand what the shenanigans accomplished vs the previous code. Stronger typing? |
|
You had the wrong types in a couple places and had to cast your way out of it, and this code also keeps the weird polymorphism restricted to the spot where we read config rather than bleeding it everywhere. Along the way we also support exiting cursorless mode for wrap, which is probably not too useful but seemed more consistent |
|
i just pushed one more commit |
|
we also have the ability now to transform the config when we create tokens, which is how we avoid that odd polymorphic type bleeding out |
Oh yeah let's do that |
Done. That's all I added. I find the new transform code for I tried to refactor it into a shared function and got lost in generics and the layers and layers of types, and eventually lost my will to continue. (I think I have a lower threshold for chasing strong typing than you do; I found the pre-shenanigans code a lot more readable. That's OK, just reflecting.) All the tests pass, and a bit of smoke testing came out ok...so I guess this is ready to ship? |
| break; | ||
| default: | ||
| action = { | ||
| name: name as any, |
There was a problem hiding this comment.
Any time you're forced to cast to any, it's a bad smell. We used to be a bit more fast and loose with our typing, and we kept getting bugs where we could trace it to this kinda thing. There's always a trade-off whether it's worth it to keep fighting with the types or give up, and I don't think the answer is always to fight till the end and have perfect strong types, but we def err more on the side of getting the right types after enough bugs that were caused by subverting the type system
| @@ -91,23 +95,43 @@ export class KeyboardConfig { | |||
| >( | |||
| tokenType: T, | |||
| sectionName: K = tokenType as unknown as K, | |||
| only?: V[], | |||
| filter?: (value: V) => boolean, | |||
There was a problem hiding this comment.
this V was the wrong type. It should have been T, because that's the input type
| @@ -52,7 +49,7 @@ export interface TokenTypeValueMap { | |||
| pairedDelimiter: SurroundingPairName; | |||
|
|
|||
| // action config section | |||
| simpleAction: SimpleKeyboardActionType; | |||
| simpleAction: KeyboardActionDescriptor; | |||
There was a problem hiding this comment.
note how we lost some type specificity here; before we knew it was a simple action
There was a problem hiding this comment.
ok, I did some cleanup, and I'm happy, so feel free to merge if you are too. I left a few comments on your original code to try to indicate why I had to do the refactoring I did
I can't say I'm totally sold on the transform thing either fwiw. I think I prefer to keep the config shorthand as close to the config processing as possible, but I don't feel super strongly. I mainly just got stuck when trying to get the types to work with the filtering approach, and decided I like the transform thing a bit better anyway for reason mentioned in previous sentence
|
I tweaked a few comments. Good to merge, I think. Since I touched last I'll let you hit the merge button. |
…ursorless-dev#2174) Fixes cursorless-dev#2117 ## Checklist - [-] I have added [tests](https://www.cursorless.org/docs/contributing/test-case-recorder/) - [-] 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: Pokey Rule <[email protected]>
Fixes #2117
Checklist