Collection item scope provider #2683

AndreasArvidsson · 2024-11-19T13:25:01Z

Related #1052

Checklist

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

phillco · 2025-01-12T19:52:39Z

data/fixtures/recorded/languages/clojure/clearEveryItem.yml

@@ -27,11 +27,10 @@ finalState:

    {
        ,
-        ;; hello


NTS: This is an example of the new code performing worse, but we understand why. This is because textual scope defines the item as:

;; hello :baz "whatever",

In this case the language does identify that the comment is not part of the item, but we are preferring the textual scope, and as above, we no longer blind take the language server's view.

This will affect every language will you have an inline comment next to an item, we just don't have many tests in our more common languages that show that. There's an issue for improving the handling of comments around items. (Might be #1770?). It's a complex change.

NB: the newer code uses more queries and predicates and less imperative TypeScript, so it's also something that would have been easier to monkey patch in the older code, although arguably, for a worse long term outcome

phillco · 2025-01-12T20:05:36Z

data/fixtures/recorded/languages/clojure/clearItem.yml

+        :bongo {
+            :foo "bar",
+            ,
+        }
    }


NTS: the old behavior was interesting: the cursor was on the comment so presumably the language server returned the entire :bongo item as the "item".

With the new behavior the textual scope includes the comment as part of the next item (see other thread), which is generally slightly worse, but in this case leads to a better outcome.

phillco · 2025-01-12T20:12:31Z

data/fixtures/recorded/languages/clojure/clearItemFine.yml

@@ -21,7 +21,7 @@ initialState:
      start: {line: 0, character: 2}
      end: {line: 0, character: 5}
 finalState:
-  documentContents: "{ :baz \"whatever\"}"
+  documentContents: "{}"


NTS: this outcome is worse but there's not much we can do about it at the current time. Problem is that closure does not use explicit list delimiters and the textual base system can obviously not figure out if a space is part of an item or a separator here. The textual system thinks the entire :foo "bar" :baz "whatever" is one "item".

Andreas' idea: allow a language to specify whether it uses explicit delimiters for lists, and if it does not, go back to the old implementation of always taking the language servers opinion.

phillco · 2025-01-12T20:14:25Z

data/fixtures/recorded/languages/clojure/chuckItemZip.yml

-        ;; hello
-    ,


Same as the other ones

phillco · 2025-01-12T20:18:37Z

data/fixtures/scopes/python/collectionItem.unenclosed.iteration3.scope

+
+
+[#2 Range] = 1:4-1:19
+      >---------------<


This syntax is indicating where Cursorless might search for items (Andreas says: that's the definition of iteration scope), although not necessarily were they could be returned. The >/< are not part of the range.

phillco · 2025-01-12T20:20:23Z

data/fixtures/scopes/python/collectionItem.unenclosed.iteration3.scope

+[#1 Range] =
+[#1 Domain] = 0:8-0:8
+          ><
+0| def foo():


NB: this range is technically empty. We don't consider it worthwhile to omit empty ranges from this test file.

Andreas says: conceivably it could be useful to be able to bootstrap an empty list by saying "pour item" and targeting this range, so empty ranges are not necessarily useless.

phillco · 2025-01-12T20:24:29Z

data/fixtures/scopes/python/collectionItem.unenclosed.iteration3.scope

+1|     global bar, baz
+
+
+[#3 Range] = 1:11-1:19


NB: this is the old range from the language server, it's not marked as such, but you know because it's identical to the old code

phillco · 2025-01-12T20:25:16Z

data/fixtures/scopes/python/collectionItem.unenclosed.iteration3.scope

+[#2 Domain] = 1:0-1:19
+  >-------------------<
+1|     global bar, baz


This domain is extra but we know from the previous code it will be eliminated when items are actually requested (global will be whittled away).

The intent of this type of test file is just to show the iteration search scopes, so this is proper.

phillco · 2025-01-12T20:40:17Z

data/fixtures/scopes/python/collectionItem.unenclosed2.scope

@@ -2,12 +2,12 @@ import foo, bar
 ---

 [#1 Content] =
-[#1 Domain] = 0:7-0:10
-         >---<
+[#1 Domain] = 0:0-0:10


NB: 1 on the right side is new; the old 1 has become 2, the old 2 has become 3

The new one here is the extra scope generated by the textual source.

As above this won't actually change behavior because when items are actually specifically requested we are going to end up the same list, import won't be included

data/fixtures/scopes/textual/collectionItem.textual.iteration6.scope

phillco · 2025-01-12T21:07:00Z

data/fixtures/scopes/textual/collectionItem.textual.iteration6.scope

+   <
+
+
+[#2 Range] = 1:4-1:6


NB: all of these ranges are from the textual source

phillco · 2025-01-12T21:18:45Z

data/fixtures/scopes/textual/collectionItem.textual8.scope

+
+[#1 Removal] = 0:1-0:4
+   >---<
+0| [1, "2, 3"]


There is specific logic to discard stuff inside of strings (text fragments)

packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/OneOfScopeHandler.ts

…andlers/OneOfScopeHandler.ts

...e/src/processTargets/modifiers/scopeHandlers/CollectionItemScopeHandler/createTargetScope.ts

…andlers/CollectionItemScopeHandler/createTargetScope.ts

changelog/2025-01-addedCollectionItemScopeProvider.md

Fix some of the clojure collection item regression from #2683 Related #2723 ## Checklist - [x] I have added [tests](https://www.cursorless.org/docs/contributing/test-case-recorder/) - [/] 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) - [/] I have not broken the cheatsheet

AndreasArvidsson added 18 commits November 18, 2024 08:57

Started working on collection item scope provider

7ad0871

More work

670afa8

Added iteration handler

3acc3a0

fix

d88f370

Update removal range

9e83dad

ignore delimiters in string

a1c3909

Updates

3ff0843

Remove old the tests

499f922

Merge branch 'main' into collectionItemScopeProvider

e4e8d7b

Added one off

2896e13

Don't yield iteration scope missing delimiters

2ba4434

Update rust

45c045b

Update ruby

3b959ae

Update ruby

ded5c79

Updates support

7fb6dc6

Update latex

78ebe56

Cleanup latex

99aa3d0

Update closuje

0004e98

AndreasArvidsson requested a review from a team as a code owner November 19, 2024 13:25

AndreasArvidsson added 11 commits November 19, 2024 16:21

Reorder handlers

a25d329

Update tests

4298d59

Added last yielded index

7630ea1

Added scope tests

1ae5c32

Merge branch 'main' into collectionItemScopeProvider

a390a19

Rename delimiter to separators

295e872

Added fall back and conditional scope handlers

d9c1937

Merge branch 'main' into collectionItemScopeProvider

2b92a3d

Bug fix

956d707

Update fixtures

b56f753

Refactor add scopes

56172e3

phillco reviewed Jan 12, 2025

View reviewed changes

phillco mentioned this pull request Jan 12, 2025

Allow languages to improve the behavior around list based scopes #2723

Open

phillco reviewed Jan 12, 2025

View reviewed changes

data/fixtures/recorded/languages/clojure/chuckItemZip.yml

Comment on lines -32 to -33

;; hello

,

Copy link

Member

phillco Jan 12, 2025

Choose a reason for hiding this comment

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

Same as the other ones

phillco reviewed Jan 12, 2025

View reviewed changes

data/fixtures/scopes/textual/collectionItem.textual.iteration6.scope Show resolved Hide resolved

phillco reviewed Jan 12, 2025

View reviewed changes

packages/cursorless-engine/src/processTargets/modifiers/scopeHandlers/OneOfScopeHandler.ts Outdated Show resolved Hide resolved

Update packages/cursorless-engine/src/processTargets/modifiers/scopeH…

e3635ba

…andlers/OneOfScopeHandler.ts

phillco reviewed Jan 13, 2025

View reviewed changes

...e/src/processTargets/modifiers/scopeHandlers/CollectionItemScopeHandler/createTargetScope.ts Outdated Show resolved Hide resolved

Update packages/cursorless-engine/src/processTargets/modifiers/scopeH…

0333308

…andlers/CollectionItemScopeHandler/createTargetScope.ts

phillco approved these changes Jan 13, 2025

View reviewed changes

Added changelog

37107fc

phillco reviewed Jan 13, 2025

View reviewed changes

changelog/2025-01-addedCollectionItemScopeProvider.md Show resolved Hide resolved

Update changelog/2025-01-addedCollectionItemScopeProvider.md

d7031b6

phillco approved these changes Jan 13, 2025

View reviewed changes

phillco enabled auto-merge January 13, 2025 18:16

[pre-commit.ci lite] apply automatic fixes

78a4516

phillco added this pull request to the merge queue Jan 13, 2025

Merged via the queue into main with commit d98bf09 Jan 13, 2025
15 checks passed

phillco deleted the collectionItemScopeProvider branch January 13, 2025 18:40

AndreasArvidsson mentioned this pull request Jan 25, 2025

Improve item collection definition for clojure #2758

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Collection item scope provider #2683

Collection item scope provider #2683

AndreasArvidsson commented Nov 19, 2024 •

edited

Loading

phillco Jan 12, 2025

phillco Jan 12, 2025

phillco Jan 12, 2025

phillco Jan 12, 2025 •

edited

Loading

phillco Jan 12, 2025

phillco Jan 12, 2025

phillco Jan 12, 2025

phillco Jan 12, 2025 •

edited

Loading

phillco Jan 12, 2025

phillco Jan 12, 2025

phillco Jan 12, 2025 •

edited

Loading

phillco Jan 12, 2025

phillco Jan 12, 2025

Collection item scope provider #2683

Collection item scope provider #2683

Conversation

AndreasArvidsson commented Nov 19, 2024 • edited Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phillco Jan 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phillco Jan 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phillco Jan 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreasArvidsson commented Nov 19, 2024 •

edited

Loading

phillco Jan 12, 2025 •

edited

Loading

phillco Jan 12, 2025 •

edited

Loading

phillco Jan 12, 2025 •

edited

Loading