Skip to content

Added text based item scope #709

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 57 commits into from
Jul 5, 2022
Merged

Added text based item scope #709

merged 57 commits into from
Jul 5, 2022

Conversation

AndreasArvidsson
Copy link
Member

@AndreasArvidsson AndreasArvidsson commented May 31, 2022

Fixes #357
Fixes #566
Fixes #441
Fixes #294

If we replace the language based implementation with the text based one like we did for string we can close a lot of language specific issues with item

Checklist

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.

As discussed, let's aim to

  • Support lists with a single item (ie no comma appears inside the parens)
  • Reuse the existing collectionItem modifier

Ideally we'd reuse existing code from generateUnmatchedDelimiters as well, but prob not a showstopper given this file is well contained, not too long, and will probably be reworked after #484

@AndreasArvidsson AndreasArvidsson marked this pull request as ready for review June 1, 2022 06:37
Base automatically changed from pokey/issue69-Support-fully-compositional-modifiers to main June 7, 2022 12:58
@pokey pokey force-pushed the textTheBasedItemScope branch from 1c75319 to 2be775a Compare June 7, 2022 18:44
@pokey
Copy link
Member

pokey commented Jun 7, 2022

Performed git surgery. Original branch at https://github.com/cursorless-dev/cursorless/tree/bak/textTheBasedItemScope; feel free to delete if this branch looks ok

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 like updated code causes type checker to be upset; I'll hold off reviewing till you get a chance to take a look

@AndreasArvidsson
Copy link
Member Author

@pokey The tests are now clearing locally

@pokey
Copy link
Member

pokey commented Jun 28, 2022

plan:

  • Remove "item" scope type from tree-sitter defs for languages that have comma-separated items
  • Leave "arg" as is in those languages
  • Duplicate "arg" tests for those languages with plaintext as lang and "item" as scope
  • Leave languages alone that don't have comma-separated lists

@AndreasArvidsson AndreasArvidsson requested a review from pokey June 29, 2022 17:12
@AndreasArvidsson
Copy link
Member Author

@pokey All languages that can are now relying on the text based implementation and I have added a bunch of new tests

@AndreasArvidsson
Copy link
Member Author

I have now added python specific implementation for item in import list

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.

Ok this looks pretty good! I made some tweaks in #823, and see inline comments. Note that tests are failing in #823 because I added a couple test cases to capture desired behaviour.
I'm obviously not in love with the code duplication with surrounding pairs, but we can revisit that when we refactor surrounding pairs as part of #484

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! Down to the last few minor comments and then let's ship it

@AndreasArvidsson
Copy link
Member Author

@pokey Your latest change requests are now implemented

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.

Ok I made a couple of final tweaks in 0791c3e. If they look good to you merge away!

@AndreasArvidsson AndreasArvidsson merged commit d9fa044 into main Jul 5, 2022
@AndreasArvidsson AndreasArvidsson deleted the textTheBasedItemScope branch July 5, 2022 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants