Skip to content
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

Support "all" modifier #473

Open
pokey opened this issue Jan 13, 2022 · 5 comments
Open

Support "all" modifier #473

pokey opened this issue Jan 13, 2022 · 5 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@pokey
Copy link
Member

pokey commented Jan 13, 2022

Similar to "every", but targets range from first to last rather than emitting one selection per item

@pokey pokey added this to To do in Expand the vocabulary via automation Jan 13, 2022
@pokey pokey added enhancement New feature or request good first issue Good for newcomers labels Jan 13, 2022
@Will-Sommers
Copy link
Collaborator

Heyo — I'm looking at this ticket as one to take and work on. If you think it will be across too much of the project, let me know.

So, taking a look at this, here's where I would start.

Processing the command on the talon side

It looks like we need to add something into cursorless-talon to pick up the "all" command just as we pick up the "every" command. I've only worked on/looked at code from the canonical command to .ts boundary. Since includeSiblings is a boolean, @pokey, I think that we'd either want:

  • Add a new modifier type to be processed within processModifier.ts
  • Change the includeSiblings boolean to encode more data, such as "single selection" or "multiple selection".

Processing the command on the cursorless side

Then within the cursorless side, support the changes above in the processModifier file, either adding a new scope type or working at the findNearestContainingAncestorNode level.

A few QQ's:

  • When working on the cursorless-talon portion of the project, I'll need symlink my ~/.talon/user/cursorless-talon to my dev folder, correct?
  • Is there a policy on adding tests for languages. Should I target every language currently supported for this?

@Will-Sommers
Copy link
Collaborator

Also, if this is blocking or not inline with the engine 2.0 work, lmk and I'll look at another ticket.

@pokey
Copy link
Member Author

pokey commented Mar 9, 2022

Yep seems like a good one! I'd proceed as follows:

  • talon side: i'd either add a bit more info to the includeSiblings field or even just add another boolean. we'll be reworking this representation so the details aren't super important. We have a good translation layer for when we change this stuff
  • extension side: I think you just need to add an if statement right before this return that unions the ranges if your boolean is set

@pokey
Copy link
Member Author

pokey commented Mar 9, 2022

Oh yeah and for tests just a couple tests is fine. It's quite orthogonal to language specifics so no need to test every language. I'd add something like:

  • One standard happy path test
  • One test of degenerate case where "every" only has one item
  • One test where they're all on a single line
  • One test in one other language

Feel free to test other corner cases that arise during impl, but otherwise the above seem like more than plenty

@Will-Sommers
Copy link
Collaborator

Cool! I'll put up a draft PR in the next few days with tests. I'm a little bit rusty with .ts, happy to take any feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
Development

No branches or pull requests

2 participants