Migrate collection item and argument scopes #2081

AndreasArvidsson · 2023-12-02T13:26:26Z

Note that we are here just migrating markdown and yaml collection. For languages like typescript I propose that the insertion delimiter is either , for an inline collection or ,\n for an vertical collection. When the edit is constructed by our destination \n gets replaced by {indent}\n or \n{indent} depending if it's before or after. In short I don't think we actually should use leading or trailing for the insertion delimiters and the destination can take care of that on its own.

To get the correct insertion delimiter we could have a conditional insertion delimiter predicate that actually uses the collection and not the item itself to determine if it's a vertical collection/insertion delimiter.

insertionDelimiter = @list.range.isSingleLine ? ", " : ",\n"

(
  (array
    (_) @collectionItem
  ) @list
  (#conditional-insertion-delimiter! @collectionItem @list ", " ",\n")
 )

Edit: Just went ahead and migrated typescript argument using the above predicate. Personally I think it turned out quite nicely.

Will fix the second task
#585

Checklist

I have added tests
[-] I have updated the docs and cheatsheet
[-] I have not broken the cheatsheet

packages/cursorless-engine/src/languages/TreeSitterQuery/QueryCapture.ts

packages/cursorless-engine/src/languages/TreeSitterQuery/queryPredicateOperators.ts

AndreasArvidsson · 2023-12-02T13:30:32Z

packages/cursorless-engine/src/processTargets/modifiers/EveryScopeStage.ts

-      ).flatMap((iterationRange) =>
-        getScopesOverlappingRange(scopeHandler, editor, iterationRange),
-      );
+      try {


getDefaultIterationRange throws an error so we never got to the legacy implementation below

Is it a NoContainingScopeError? If so I'd argue we should look for that error specifically

True. Fixed now

Just commenting from the future to say that we removed this :D #2740

packages/cursorless-vscode-e2e/src/suite/fixtures/recorded/languages/markdown/chuckItem2.yml

packages/cursorless-vscode-e2e/src/suite/fixtures/recorded/languages/yaml/changeEveryItem3.yml

queries/javascript.core.scm

queries/markdown.scm

pokey · 2023-12-04T13:16:47Z

Pasting from slack:

I think insertion range is the wrong name tho because that’s too easily confused with “to” destination, ie that’s what I would assume “insertion range” means. It’s unfortunate they’re not equal
Maybe insertionBoundary? adjacentDestinations? beforeAfterDestination?
I mean tbh the clearest would be separate positions for each. Using a range is kind of a shortcut. And in this case we only need one side I believe, in which case just beforeDestionation or destinationBefore. I think that’s super clear
Then we could have destinationTo for the “to” destination, eg for Support destination pseudo-scopes #1631
for destinationBefore / destinationAfter, they’d be zero-width
I think that’s nice and consistent - Or we could have it be an arg, eg (#destination! "before" @foo)
all that being said, I do wonder if what we really want is something semantically richer, along the lines of:
- eg markdown / yaml lists are newline-delimited, indentation-nested, with given start of line marker
I feel like anything less is kind of a stopgap that we’re going to end up having to work around

to discuss Plan to discuss at meet-up

AndreasArvidsson · 2023-12-04T14:19:29Z

Another thing I just noticed when it comes to the removal range of markdown and yaml items is that the "insertion range" should be used for removal instead of the content range. You of course want to remove the leading dash.

I would argue that we here have two different content ranges actually. We have the outer range containing things like the dash or asterisks. This range is what we use when we remove or when we create insertions before or after. We then have the inner content range which actually has the text content. This is what we use for things like copy, change and so on.

We definitely need to discuss this tomorrow

Edit: We could use this "extended content range" in different actions. For example copy and bring might actually include the prefix while change and take doesn't. That kind of thing.

Edit2: This issue could benefit from two different content ranges: #2014

AndreasArvidsson · 2023-12-04T17:42:28Z

Another thing that just struck me(the item patterns are getting quite crowded) is why do we need to give a start and an end to the leading and trailing ranges? shouldn't they always terminate at the content range(or extended content range)? I think we can just use @_.leading and @_.trailing with optional .startOf/.endOf. We already know that the leading terminates at the start of the content range and the trailing starts at the end of the content range.

pokey · 2023-12-07T13:24:55Z

Filed #2106 to capture our discussion

AndreasArvidsson · 2023-12-07T14:19:56Z

@pokey Updated according to today's meet up

pokey

Looking good! I almost see a way to keep the prefix field out of the Target interface, but prob better to let it leak out for now just to get this thing out

I left one comment; if we don't get an answer from @josharian on #2108 in the next 24hrs let's push forward anyway; I agree it's a nice slick syntax

pokey · 2023-12-07T16:54:49Z

.../src/processTargets/modifiers/scopeHandlers/TreeSitterScopeHandler/TreeSitterScopeHandler.ts

+    );
+    const prefixRange =
+      rawPrefixRange != null
+        ? new Range(rawPrefixRange.start, contentRange.start)


We should prob only do this if we decide to go ahead with #2108

I really don't like having to do unnecessary syntax, but sure we can await that discussion.

pokey · 2023-12-07T16:56:35Z

packages/cursorless-engine/src/processTargets/modifiers/EveryScopeStage.ts

-      ).flatMap((iterationRange) =>
-        getScopesOverlappingRange(scopeHandler, editor, iterationRange),
-      );
+      try {


Is it a NoContainingScopeError? If so I'd argue we should look for that error specifically

pokey

🎉

Migrate markdown and yaml collection item scope

1046522

AndreasArvidsson requested a review from pokey as a code owner December 2, 2023 13:26

AndreasArvidsson mentioned this pull request Dec 2, 2023

Use arbitrary capture as insertion delimiter #2080

Closed

2 tasks

AndreasArvidsson commented Dec 2, 2023

View reviewed changes

packages/cursorless-engine/src/languages/TreeSitterQuery/QueryCapture.ts Outdated Show resolved Hide resolved

AndreasArvidsson commented Dec 2, 2023

View reviewed changes

packages/cursorless-engine/src/languages/TreeSitterQuery/queryPredicateOperators.ts Outdated Show resolved Hide resolved

AndreasArvidsson commented Dec 2, 2023

View reviewed changes

packages/cursorless-vscode-e2e/src/suite/fixtures/recorded/languages/markdown/chuckItem2.yml Show resolved Hide resolved

AndreasArvidsson commented Dec 2, 2023

View reviewed changes

packages/cursorless-vscode-e2e/src/suite/fixtures/recorded/languages/yaml/changeEveryItem3.yml Show resolved Hide resolved

AndreasArvidsson added the to discuss Plan to discuss at meet-up label Dec 2, 2023

auscompgeek added the scope-migration Migrating scopes to next-gen scope implementation label Dec 3, 2023

AndreasArvidsson added 8 commits December 3, 2023 04:42

cleanup

4daa292

cleanup

f90eedd

cleanup

6ddfe3f

migrate typescript argument

32e2277

Added the comment

aca116e

Added comments

7420dec

cleanup

4dbd9f5

cleanup

bb0e6a6

AndreasArvidsson changed the title ~~Migrate markdown and yaml collection item scope~~ Migrate collection item and argument scopes Dec 3, 2023

Exclude comments

b0e3331

pokey reviewed Dec 3, 2023

View reviewed changes

queries/javascript.core.scm Outdated Show resolved Hide resolved

pokey reviewed Dec 3, 2023

View reviewed changes

queries/markdown.scm Outdated Show resolved Hide resolved

AndreasArvidsson added 2 commits December 3, 2023 20:47

Added tests

6fd01d1

Introduced insertion range

45f96b1

Fix mark down and yaml it them removal range

1126918

Change insertion range to extended content range

2878f44

pokey mentioned this pull request Dec 7, 2023

@scope.prefixRange target attribute #2106

Open

AndreasArvidsson added 5 commits December 7, 2023 14:26

Merge branch 'main' into collectionItem

685af60

remove unused typescript file

1956c86

use prefix range

c85623e

cleanup

17657f3

cleanup

d484c2b

pokey added 2 commits December 7, 2023 16:51

Docs

265aee8

tweak

11e4ebb

pokey reviewed Dec 7, 2023

View reviewed changes

Check error type

33fdd5a

pokey approved these changes Dec 8, 2023

View reviewed changes

pokey added this pull request to the merge queue Dec 8, 2023

Merged via the queue into main with commit 40a6fee Dec 8, 2023
13 checks passed

pokey deleted the collectionItem branch December 8, 2023 21:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate collection item and argument scopes #2081

Migrate collection item and argument scopes #2081

AndreasArvidsson commented Dec 2, 2023 •

edited

Loading

AndreasArvidsson Dec 2, 2023 •

edited

Loading

pokey Dec 7, 2023

AndreasArvidsson Dec 7, 2023

phillco Jan 21, 2025

pokey commented Dec 4, 2023

AndreasArvidsson commented Dec 4, 2023 •

edited

Loading

AndreasArvidsson commented Dec 4, 2023

pokey commented Dec 7, 2023

AndreasArvidsson commented Dec 7, 2023

pokey left a comment

pokey Dec 7, 2023

AndreasArvidsson Dec 7, 2023

pokey Dec 7, 2023

pokey left a comment

Migrate collection item and argument scopes #2081

Migrate collection item and argument scopes #2081

Conversation

AndreasArvidsson commented Dec 2, 2023 • edited Loading

Checklist

AndreasArvidsson Dec 2, 2023 • edited Loading

Choose a reason for hiding this comment

pokey Dec 7, 2023

Choose a reason for hiding this comment

AndreasArvidsson Dec 7, 2023

Choose a reason for hiding this comment

phillco Jan 21, 2025

Choose a reason for hiding this comment

pokey commented Dec 4, 2023

AndreasArvidsson commented Dec 4, 2023 • edited Loading

AndreasArvidsson commented Dec 4, 2023

pokey commented Dec 7, 2023

AndreasArvidsson commented Dec 7, 2023

pokey left a comment

Choose a reason for hiding this comment

pokey Dec 7, 2023

Choose a reason for hiding this comment

AndreasArvidsson Dec 7, 2023

Choose a reason for hiding this comment

pokey Dec 7, 2023

Choose a reason for hiding this comment

pokey left a comment

Choose a reason for hiding this comment

AndreasArvidsson commented Dec 2, 2023 •

edited

Loading

AndreasArvidsson Dec 2, 2023 •

edited

Loading

AndreasArvidsson commented Dec 4, 2023 •

edited

Loading