Skip to content

Add "branch" scope type #1149

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 17 commits into from
Jan 3, 2023
Merged

Add "branch" scope type #1149

merged 17 commits into from
Jan 3, 2023

Conversation

pokey
Copy link
Member

@pokey pokey commented Nov 18, 2022 •
edited
Loading

Checklist

Python Rust Java Typescript C++ C#
Switch-case ✅ ✅ ✅ ✅
If-else ✅ ✅ ✅
Try-catch ✅ N/A ✅
Ternary ✅ ✅ ✅

Co-authored-by: Michael Doronin warrior2031@mail.ru

This was referenced Nov 18, 2022
Closed
Closed
@pokey pokey force-pushed the pokey/add-branch-scope-type branch from 78fa594 to d6fe259 Compare November 18, 2022 17:34
@pokey pokey force-pushed the pokey/add-subject-scope-type branch from 252ca38 to bbc1ca4 Compare November 18, 2022 18:15
@pokey pokey force-pushed the pokey/add-branch-scope-type branch 2 times, most recently from a3e4576 to 90619bc Compare November 18, 2022 18:23
@pokey pokey changed the base branch from pokey/add-subject-scope-type to main November 18, 2022 18:24
@pokey pokey force-pushed the pokey/add-branch-scope-type branch 3 times, most recently from 7350720 to adf7c5e Compare November 18, 2022 18:46
@pokey pokey force-pushed the pokey/add-branch-scope-type branch from adf7c5e to 7ba66c5 Compare November 18, 2022 18:55
@pokey pokey changed the title Add branch scope type Add "branch" scope type Dec 14, 2022
@pokey pokey mentioned this pull request Dec 14, 2022
Open
@pokey
Copy link
Member Author

pokey commented Dec 15, 2022

Closing until someone has bandwidth to finish this PR

@pokey pokey closed this Dec 15, 2022
@pokey
Copy link
Member Author

pokey commented Dec 15, 2022

After discussion in meet-up:

Just kidding; let's get this thing in for the languages that exist here. more useful to at least have it functional in some places

@pokey pokey reopened this Dec 15, 2022
@pokey pokey self-assigned this Dec 15, 2022
@pokey pokey force-pushed the pokey/add-branch-scope-type branch from 7f8ad4c to 20f4c8b Compare December 22, 2022 12:13
@pokey pokey marked this pull request as ready for review December 22, 2022 15:44
@auscompgeek
Copy link
Member

auscompgeek commented Dec 23, 2022 •
edited by pokey
Loading

We should also test:

  • Python for/else and while/else
  • Rust if let and let/else

auscompgeek
auscompgeek reviewed Dec 23, 2022
View reviewed changes
src/test/suite/fixtures/recorded/languages/python/clearBranch14.yml
marks: {}
finalState:
documentContents: |-

Copy link
Member

Choose a reason for hiding this comment

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

It feels a bit weird for branch to work on the try block itself 🤷

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I'm not entirely sure what the use case for that is tbh 😅, but seemed consistent, and maybe someone would want to use it with the copy action?

@pokey
Copy link
Member Author

pokey commented Dec 23, 2022

  • Rust if let and let/else

Unfortunately that is blocked on #1201 😕

@pokey pokey requested a review from auscompgeek December 23, 2022 12:26
@auscompgeek
Copy link
Member

Unfortunately that is blocked on #1201 😕

Ah, I was afraid as such.

auscompgeek
auscompgeek previously approved these changes Dec 23, 2022
View reviewed changes
Copy link
Member

@auscompgeek auscompgeek left a comment

Choose a reason for hiding this comment

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

I think the implicit "this" change got cherry-picked into this branch for some reason, but otherwise LGTM.

I presume it should be trivial to reuse the if/else matchers for Java and C++ like we did for Rust, and I hope the same for try/catch, so might be worth doing it here anyway.

cursorless-talon/src/modifiers/scopes.py
@@ -46,6 +46,7 @@
"selector": "selector",
"state": "statement",
"string": "string",
"branch": "branch",
Copy link
Member

Choose a reason for hiding this comment

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

nit: any reason this is listed in this order in particular?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really. I should prob file a follow-up to alphabetise; didn't want to mess with the diff

@pokey
Copy link
Member Author

pokey commented Jan 3, 2023

I think the implicit "this" change got cherry-picked into this branch for some reason, but otherwise LGTM.

Strange. That's already merged into main anyway, but looks like the parentage is strange here; I'll have a look

I presume it should be trivial to reuse the if/else matchers for Java and C++ like we did for Rust, and I hope the same for try/catch, so might be worth doing it here anyway.

Unfortunately not 😕. Java, C#, and C++ don't have a special node for else clauses, so the logic gets more complicated. Should be doable, but not just a quick afterthought like one would hope. I'd be inclined to leave for follow-up work

@pokey pokey force-pushed the pokey/add-branch-scope-type branch from 397d6ab to 643090e Compare January 3, 2023 12:35
@pokey pokey merged commit f8a03dd into main Jan 3, 2023
@pokey pokey deleted the pokey/add-branch-scope-type branch January 3, 2023 15:21
cursorless-bot pushed a commit that referenced this pull request Jan 3, 2023
@pokey
Add "branch" scope type (#1149)
a07b016
thetomcraig-aya pushed a commit to thetomcraig/cursorless that referenced this pull request Mar 27, 2024
@pokey @cursorless-bot
Add "branch" scope type (cursorless-dev#1149)
014ebb3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@auscompgeek auscompgeek auscompgeek approved these changes

@AndreasArvidsson AndreasArvidsson Awaiting requested review from AndreasArvidsson

Assignees

@pokey pokey

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pokey @auscompgeek