Skip to content

implement 'short paint' scope type #737

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

Merged
merged 43 commits into from
Jul 3, 2022
Merged

Conversation

AndrewDant
Copy link
Collaborator

@AndrewDant AndrewDant commented Jun 5, 2022

Closes #587

Checklist

@Will-Sommers
Copy link
Collaborator

Looking good!

Copy link
Member

@pokey pokey left a comment

Choose a reason for hiding this comment

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

Nice! Would be good to add a couple tests that exercise the "paint" aspect—all your tests just focus on the "small" part. So I'd have the following tests, each time testing both "clear" and "chuck":

  • "hello-world"
  • "hello-world testing"
  • "some hello-world test"

@pokey
Copy link
Member

pokey commented Jun 6, 2022

Also, re excluding other delimiters, it's a good question. I prob wouldn't want to exclude square brackets, because I'd prob want to be able to grab foo[0]. Maybe let's keep it to quotes for now?

@AndrewDant
Copy link
Collaborator Author

Also, re excluding other delimiters, it's a good question. I prob wouldn't want to exclude square brackets, because I'd prob want to be able to grab foo[0]. Maybe let's keep it to quotes for now?

I'm content with that.

@AndrewDant AndrewDant marked this pull request as ready for review June 6, 2022 12:52
@Will-Sommers
Copy link
Collaborator

Heyo @AndrewDant — happy to walk through this merge conflict if you end up getting stuck. I had to do it for a few PRs already. Excited for this modifier to come out!

@pokey
Copy link
Member

pokey commented Jun 22, 2022

Should we switch this one to "short paint"? That will be more consistent with "long head" when we add that

Copy link
Member

@pokey pokey left a comment

Choose a reason for hiding this comment

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

Interesting direction. I still don't totally understand how "every" should work, but I left a bunch of minor comments.

Fwiw, the advantage of the original approach is that "every" becomes trivial, though I think in that case I'd still want "every" to expand to nearest containing string if input target is empty

@AndreasArvidsson
Copy link
Member

AndreasArvidsson commented Jun 28, 2022

@AndrewDant To fix the bug with paint just replace range.end.isBeforeOrEqual(end) with range.start.isBeforeOrEqual(end) at RegexStage.ts:L41

@pokey
Copy link
Member

pokey commented Jun 28, 2022

Also a couple more tests to add:

  • "change every small paint pair paren"
  • "change every small paint inside paren"

Copy link
Member

@pokey pokey left a comment

Choose a reason for hiding this comment

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

LGTM with minor comment!

@AndrewDant
Copy link
Collaborator Author

I've added more tests, we might be ready to go now but let me know if I missed anything.

Copy link
Member

@pokey pokey left a comment

Choose a reason for hiding this comment

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

Looks good! Ship it 🙌

@pokey pokey changed the title implement 'small paint' scope type implement 'short paint' scope type Jul 3, 2022
@pokey pokey merged commit 492e56f into cursorless-dev:main Jul 3, 2022
cursorless-bot pushed a commit that referenced this pull request Jul 3, 2022
* implement 'small paint' scope type

* additional test with white space on each side

* updates to get small paint working besides removal range

* stage refactors recommended during pair programming

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* revert some refactors after merging

* Updates small paint to stop on any surrounding pairs' delimiter

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* some additional small paint tests

* rename small paint stage

* remove unnecessary changed

* update default phrase from small paint to short paint

* update short paint every behavior

* update short paint to call process surrounding pair directly

* fix short paint stage capitalization

* fix short paint modifier capitalization

* update small paint  every tests and scope type in tests

* refactor  short paint

* Switch to strong containment implementation

* fix bug with paint selection type

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* fix a reference to short paint stage name

* remove outdated tests

* boundedNonWhitespaceStage -> BoundedNonWhitespaceStage

* Cleanup

* paint and short paint tests

* better change every paint pair tests

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Andreas Arvidsson <[email protected]>
Co-authored-by: Pokey Rule <[email protected]>
thetomcraig-aya pushed a commit to thetomcraig/cursorless that referenced this pull request Mar 27, 2024
* implement 'small paint' scope type

* additional test with white space on each side

* updates to get small paint working besides removal range

* stage refactors recommended during pair programming

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* revert some refactors after merging

* Updates small paint to stop on any surrounding pairs' delimiter

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* some additional small paint tests

* rename small paint stage

* remove unnecessary changed

* update default phrase from small paint to short paint

* update short paint every behavior

* update short paint to call process surrounding pair directly

* fix short paint stage capitalization

* fix short paint modifier capitalization

* update small paint  every tests and scope type in tests

* refactor  short paint

* Switch to strong containment implementation

* fix bug with paint selection type

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* fix a reference to short paint stage name

* remove outdated tests

* boundedNonWhitespaceStage -> BoundedNonWhitespaceStage

* Cleanup

* paint and short paint tests

* better change every paint pair tests

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

Successfully merging this pull request may close these issues.

Add "short paint" modifier
4 participants