Skip to content

Allow using "this" to refer to current token with empty selection #1094

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 64 commits into from
Dec 22, 2022
Merged

Allow using "this" to refer to current token with empty selection #1094

merged 64 commits into from
Dec 22, 2022

Conversation

AndreasArvidsson
Copy link
Member

@AndreasArvidsson AndreasArvidsson commented Oct 26, 2022 •
edited by pokey
Loading

@AndreasArvidsson AndreasArvidsson added the to discuss Plan to discuss at meet-up label Oct 26, 2022
pokey
pokey reviewed Oct 31, 2022
View reviewed changes
Copy link
Member

@pokey pokey left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good with some minor comments. Also worth adding tests for:

  • "bring this to bat"
  • "bring bat to this"
  • "chuck this"

@AndreasArvidsson AndreasArvidsson removed the to discuss Plan to discuss at meet-up label Oct 31, 2022
@AndreasArvidsson
Copy link
Member Author

@pokey Ready!

@pokey
Copy link
Member

pokey commented Nov 8, 2022

Thinking about it more, I don't think we should have special cases for ranges. It is confusing if "bring this to bat" behaves differently from "take this past bat". Happy to discuss at meet-up if you prefer the special case for ranges

@AndreasArvidsson AndreasArvidsson mentioned this pull request Nov 8, 2022
Open
5 tasks
@AndreasArvidsson
Copy link
Member Author

Thinking about it more, I don't think we should have special cases for ranges. It is confusing if "bring this to bat" behaves differently from "take this past bat". Happy to discuss at meet-up if you prefer the special case for ranges

I'm not sure either. Lets every discussion.

@pokey pokey added the to discuss Plan to discuss at meet-up label Nov 10, 2022
@pokey
Copy link
Member

pokey commented Nov 15, 2022 •
edited by AndreasArvidsson
Loading

Update based on discussion:

  • No special cases for range targets
  • Make sure that "take past air" uses implicit target for anchor
  • Update tests, potentially splitting some of them into up to three tests:
    • using "just" to get old behaviour
    • using implicit target to get old behaviour
    • new behaviour

@pokey pokey changed the base branch from pokey/more-api-surface-type-cleanup to main December 17, 2022 15:16
@pokey pokey changed the base branch from main to pokey/more-api-surface-type-cleanup December 17, 2022 15:18
@pokey
Copy link
Member

pokey commented Dec 17, 2022

I guess maybe it's ok? It's causing 4 CI failures...

Base automatically changed from pokey/more-api-surface-type-cleanup to main December 20, 2022 14:13
pokey
pokey previously approved these changes Dec 20, 2022
View reviewed changes
Copy link
Member

@pokey pokey left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Ok I think this one is good to go; I'll give you a chance to have a quick look if you get a minute @AndreasArvidsson

@pokey
Copy link
Member

pokey commented Dec 20, 2022 •
edited
Loading

Update from meet-up:

  • Let's guard this one with a setting: cursorless.experimental.featureFlags.emptyCursorTokenExpansion
  • Let's add tests that exercise this setting

See #1094 (comment)

@AndreasArvidsson
Copy link
Member Author

@pokey Looks good

@pokey
Copy link
Member

pokey commented Dec 22, 2022

I found myself wanting this one again today; I say let's ship it without feature flag. If we decide it's a bad idea down the road, we can always:

  1. move it behind a feature flag, then
  2. have default behaviour (no feature flag) be to show a warning, pointing them to doc for enabling feature flag

@AndreasArvidsson AndreasArvidsson merged commit 1a06165 into main Dec 22, 2022
@AndreasArvidsson AndreasArvidsson deleted the expand_empty_selection_to_token branch December 22, 2022 20:04
cursorless-bot pushed a commit that referenced this pull request Dec 22, 2022
@AndreasArvidsson @pokey @pre-commit-ci
Allow using "this" to refer to current token with empty selection (#1094
4d20d6f 
)

- Fixes #1043
- Fixes #1141 
- Depends on #1187

## 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)

Co-authored-by: Pokey Rule <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
pokey added a commit that referenced this pull request Dec 23, 2022
@AndreasArvidsson @pokey @pre-commit-ci
Allow using "this" to refer to current token with empty selection (#1094
397d6ab 
)

- Fixes #1043
- Fixes #1141 
- Depends on #1187

## 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)

Co-authored-by: Pokey Rule <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
pokey added a commit that referenced this pull request Jan 5, 2023
@pokey
Fix modal keyboard interface broken by #1094
4cc71da
pokey added a commit that referenced this pull request Jan 5, 2023
@pokey
Fix modal keyboard interface broken by #1094
e984d36
pokey added a commit that referenced this pull request Jan 6, 2023
@pokey
Fix modal keyboard interface broken by #1094 (#1213)
c2e930f
thetomcraig-aya pushed a commit to thetomcraig/cursorless that referenced this pull request Mar 27, 2024
@AndreasArvidsson @pokey
@pre-commit-ci @cursorless-bot
Allow using "this" to refer to current token with empty selection (cu…
85eff4d 
…rsorless-dev#1094)

- Fixes cursorless-dev#1043
- Fixes cursorless-dev#1141 
- Depends on cursorless-dev#1187

## 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)

Co-authored-by: Pokey Rule <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@auscompgeek auscompgeek auscompgeek left review comments

@pokey pokey pokey approved these changes

Assignees

@pokey pokey

Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@AndreasArvidsson @pokey @auscompgeek