Scope tests #2053

AndreasArvidsson · 2023-11-22T10:30:57Z

Started implementing a new scope handler test representation

[Content]
const name = "Hello world";
              ^^^^^^^^^^^^^

Originally I was thinking of doing ^ for each character. That worked fine until @pokey asked the question about empty ranges which we actually has in a few places. So I switch to a multiline representation of [---] where the empty range would be [].

[Content]
 function myFunk() {
[-------------------
 
 }
 -]

I do wonder if we want to start multiline ranges above the source code and end it below. Makes it so the code is actually inside the range endings.

[Content]
[-------------------
 function myFunk() {
 
 }
 -]

Relevant issues
#1524
#2010

Checklist

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

…to scope_tests

ests' of github.com:

pokey · 2023-11-22T14:21:29Z

So I switch to a representation of [---] where the empty range would be [].

As discussed in original issue, we'd like to remove the brackets when it's a single-line range. Why not just use the originally suggested format, modulo the notion that we do one range type per section?

I do wonder if we want to start multiline ranges above the source code and end it below. Makes it so the code is actually inside the range endings.

I think that makes sense if there are only two lines, but what happens if there are more lines? Do the middle ones go above or below? I think I'd lean toward consistency

AndreasArvidsson · 2023-11-22T14:23:21Z

So I switch to a representation of [---] where the empty range would be [].

As discussed in original issue, we'd like to remove the brackets when it's a single-line range. Why not just use the originally suggested format, modulo the notion that we do one range type per section?

It's just a question about how we would represent an empty range?

I do wonder if we want to start multiline ranges above the source code and end it below. Makes it so the code is actually inside the range endings.

I think that makes sense if there are only two lines, but what happens if there are more lines? Do the middle ones go above or below? I think I'd lean toward consistency

the lines outside of the range would not be affected by this.

[Content]
 // before
 function myFunk() {
[-------------------

 }
 -]
 // after

vs

[Content]
 // before
[-------------------
 function myFunk() {

 }
 -]
 // after

pokey · 2023-11-22T14:34:37Z

It's just a question about how we would represent an empty range?

By "original format" I mean that specified in the Examples section of the description of #1524, just modified so that

we have unmodified document at the top before ---
we reproduce document once per range type per target
instead of varying the annotation style by range type, we always use the { / ^ style (but note that we drop the { for single-line targets, as per initial spec, in contrast to your suggestion here). I much prefer ^ to -, but I guess if we adopt your other suggestion then the start arrows are pointing the wrong direction 😅

the lines outside of the range would not be affected by this.

no I don't mean lines outside range, I mean lines inside range. But I guess we can just leave out the annotations there because we know the range is contiguous? eg

[content]
[-----------
 if (true) {
     const foo = "bar";
 }
 -]

I think it's an interesting idea, tho I wonder if we drop a bunch of the -'s in this case because they're a bit misleading, because then there's no dashes above or below the other internal lines. Maybe we just drop the -s altogether and just use [ / ] (though I think I might prefer curlies)

I also wonder if there's some ambiguity here as there's no marker distinguishing annotation lines from code lines, and we're no longer following alternation convention

AndreasArvidsson · 2023-11-22T14:36:45Z

^^^^ for single line and [-- or {--} for multiline sounds reasonable. Would we represent an empty range like [] or {}?

[content]
[-----------
 if (true) {
     const foo = "bar";
 }
 -]

That was what I was thinking. As you said it's a contiguous range. We can of course extend the top line all the way to the right if we think that is a better indicator?

edit: This format also works out quite nicely when we have leading and trailing delimiters with empty lines

[#2 Leading delimiter: Content]
[
 
 aaa
 ]

 
[#2 Trailing delimiter: Content]
 
   [
 aaa
 
 ]

AndreasArvidsson · 2023-11-22T15:02:33Z

@pokey I have updated the fixture files. [ vs { is something we can discuss further. Does the format look okay otherwise?

pokey

Just had a quick look through a few of the fixture files, and left a few comments. Lmk if you want me to review the rest

packages/cursorless-vscode-e2e/src/suite/fixtures/scopes/javascript/assignment.name.scope

packages/cursorless-vscode-e2e/src/suite/fixtures/scopes/javascript/assignment.value.scope

packages/cursorless-vscode-e2e/src/suite/fixtures/scopes/javascript/function.scope

packages/cursorless-vscode-e2e/src/suite/fixtures/scopes/javascript/line.scope

AndreasArvidsson · 2023-11-30T19:40:07Z

Do we want iteration scope in these fixtures? Or maybe that should be a separate fixture?

pokey · 2023-11-30T19:56:14Z

Hmm. Not sure. Maybe separate fixture? I wonder if iteration scope for each scope should be its own facet. Not a bad idea, to make people think about iteration scope for scopes for which it matters

AndreasArvidsson · 2023-11-30T20:05:01Z

Yes I was thinking of separate facets. Probably a good idea?

pokey · 2023-11-30T21:22:31Z

Yeah let's go with separate facets 👍

AndreasArvidsson · 2023-12-01T04:16:15Z

@pokey Iterations scope facet and fixtures added

pokey · 2023-12-01T09:21:33Z

packages/common/src/scopeSupportFacets/html.ts

+
+export const htmlSupport: LanguageScopeSupportFacetMap = {
+  ["key.attribute"]: supported,
+  ["tags.element"]: supported,


Why the .element here?

We might have tags in another context in the future. I'm just trying to be thorough.

Hmm. I guess it's a bit more future proof, tho feels like following this convention to the letter might result in some awkward names. Idk cc/ @josharian

idk either

how immutable do we need these facets to be? can we lower the cost of mistakes sufficiently that it doesn't matter much?

Were gonna have a lot of files with these facets as their name so it's not gonna be that easy to change them, but also definitely not impossible.

I guess a simple rename should handle all the typescript refs, and then we could easily write a script that moves the test fixtures

A bit annoying having to do a script, but definitely doable

Update from meet-up: let's go with just tags. It's ok to have the general thing and then sub-facets as well

packages/cursorless-vscode-e2e/src/suite/fixtures/scopes/html/tags.element.scope

pokey · 2023-12-01T09:24:13Z

...ges/cursorless-vscode-e2e/src/suite/fixtures/scopes/javascript/value.mapPair.iteration.scope

+[#1 Range] =
+[#1 Domain] = 0:0-0:14
+0| { value: 123 }
+  >--------------<


Huh. Technically this iteration scope is not for this facet. Not sure there's much we can do about that tho 🤷‍♂️

I don't think so. This would be a reason to have more fine grained scopes as well.

Yeah fair point. I wonder how many of our facets should really be their own scopes 🤔. I guess nice thing bout facets is it makes it easier for us to switch them to their own scopes later because they're explicitly tagged

Probably quite a few.

I think one indicator that a scope maybe should split is that we have multiple iteration scopes as in the above example. For example all the different types of ways you can do functions in javascript is still the same function scope, but a value in a map pair is not really the same thing as a return statement value and they do have different iteration scopes. This is definitely going to be on a case by case basis, but I think that having multiple iteration scopes for the same scope is a bit of a warning sign.

packages/common/src/scopeSupportFacets/getLanguageScopeSupport.ts

AndreasArvidsson · 2023-12-05T17:34:43Z

@pokey Updated after today's meeting discussion

pokey

Couple more minor comments

.pre-commit-config.yaml

packages/common/src/scopeSupportFacets/textual.ts

Co-authored-by: Pokey Rule <[email protected]>

pokey

I find I'm not in love with label and examples fields of facet info. Shall we make them optional and delete them where they're unhelpful? I'm happy to do that as I have opinions about where they're unhelpful 😅

pokey · 2023-12-06T13:42:19Z

packages/common/src/scopeSupportFacets/javascript.ts

+  ["value.assignment"]: supported,
+
+  ["key.attribute"]: notApplicable,
+  ["tags"]: notApplicable,


this isn't true. JSX is valid in JS. Shall we just mark it unsupported / leave it undefined for now?

I was thinking that we should test all jsx under javascriptreact and not javascript. I would leave it undefined if anything. Saying that it's unsupported is not true either.

Hmm. Probably fine for now. At some point we should prob figure out how to automatically run tests for all langs where they're applicable, eg run our js tests in ts, etc. We could maybe make subsets of languages and then define a way to import tests from one language to another. But we can do that in a follow-up

pokey · 2023-12-06T13:48:58Z

packages/common/src/scopeSupportFacets/textualScopeSupportFacetInfos.ts

+    label: "Word",
+    description: "A single word in a token",
+    scopeType: "word",
+    examples: ["foo_bar", "fooBar"],


These examples are pretty misleading. If we're going to take the example thing seriously, we need to figure out a way to demarcate substrings. I do question how useful / ergonomic it is to just drop these examples into strings. The examples from test casees are already there and likely to be far more useful

I was thinking examples as a very general guide. If you want details you she'd look at the actual fixtures

pokey · 2023-12-06T13:51:43Z

packages/common/src/scopeSupportFacets/textualScopeSupportFacetInfos.ts

+    label: "Line",
+    description: "A single line in the document",
+    scopeType: "line",
+    examples: ["foo"],


this is unhelpful in the other direction. Really questioning the utility of these examples 🤔

If you don't like them we can't ditch them

pokey · 2023-12-06T13:53:44Z

packages/common/src/scopeSupportFacets/scopeSupportFacets.types.ts

+export interface ScopeSupportFacetInfo {
+  readonly label: string;
+  readonly description: string;
+  readonly scopeType: SimpleScopeTypeType;


What's going to happen when we want to test "round"? That's not a simple scope type

I just copied the interface from your issue. We can make it a scope type instead?

Maybe ok for the time being. I wonder if we create a utility function to make it more ergonomic once we need to support complex scopes.

pokey · 2023-12-06T13:59:07Z

packages/common/src/scopeSupportFacets/scopeSupportFacetInfos.ts

+    label: "Attributes key",
+    description: "Key(LHS) of an attribute",
+    scopeType: "collectionKey",
+    examples: ['id="root"'],


As in other examples, this would be more helpful if there were a way to put it in context, but a flat string is not a particularly comfortable way to do that

Absolutely.

pokey · 2023-12-06T13:59:58Z

packages/common/src/scopeSupportFacets/scopeSupportFacetInfos.ts

+    description: "Iteration of map pair keys",
+    scopeType: "collectionKey",
+    isIteration: true,
+    examples: ["{ value: 0 }"],


This shouldn't include the curly brackets, but then it becomes pretty unclear what's going on here. I guess we could use multiple?

The examples are not the content range. It's just example of the type of code that would have this facet

pokey · 2023-12-06T14:01:27Z

packages/common/src/scopeSupportFacets/scopeSupportFacetInfos.ts

+    examples: ['id="root"'],
+  },
+  ["key.mapPair"]: {
+    label: "Map key",


These labels feel kinda useless / arbitrary and a bit redundant with the description. I wonder if we just autogenerate these from the id like we do for human-readable scope types. Ie key.mapPair => "key (map pair)"

Sounds good to me

pokey · 2023-12-06T14:38:33Z

packages/common/src/scopeSupportFacets/scopeSupportFacets.types.ts

@@ -0,0 +1,82 @@
+import { SimpleScopeTypeType } from "../types/command/PartialTargetDescriptor.types";
+
+const scopeSupportFacets = [


I do feel like all of these could be their own fine-grained scope. I wonder if we think about moving in that direction sooner rather than later

I don't quite follow what you're suggesting? What exactly do you want to make more fine grained? I definitely prefer them fine grained

Eg key.mapPair, key.attribute, etc could all be their own scopes. I think every facet here should probably just be a new fine-grained scope. Not something to change for this PR just an observation. Makes me question the notion of facets entirely, but they're undoubtedly a step forward so nothing to get in the way of this PR

Totally agree

AndreasArvidsson · 2023-12-06T15:19:46Z

I would be happy to remove label and examples, but I don't think we should make them optional.

AndreasArvidsson and others added 22 commits November 22, 2023 10:00

Started working on scope tests

6ac7ea6

Extend scope provider interface

9bc6d8a

Add insertion delimiter

8326276

Added additional test

5feb0cf

[pre-commit.ci lite] apply automatic fixes

6cd7cc0

Trailing newline

eac4b30

Merge branch 'scope_tests' of github.com:cursorless-dev/cursorless in…

fcef5ec

…to scope_tests

Added example with multipole scopes

0348ae4

Added function

dde8a77

Changed representation

9f79367

[pre-commit.ci lite] apply automatic fixes

33263bb

Change file ending

91a5fc2

[pre-commit.ci lite] apply automatic fixes

9123d4b

number

ec2004c

Merge branch 'scope_t

a1e62ea

ests' of github.com:

fix

fcefb46

[pre-commit.ci lite] apply automatic fixes

191454f

test

5814cd5

fix

e3753b3

Added paragraph

7dcde95

fix

c5d8fdb

try

071e815

fix

c6b8aaf

New format

b756723

pokey reviewed Nov 22, 2023

View reviewed changes

Added iteration scope

b8c2ade

Up date javascript support

5f1d178

pokey reviewed Dec 1, 2023

View reviewed changes

AndreasArvidsson added 2 commits December 1, 2023 12:46

Update number serialization

93d36d0

Update target number

b8a80a8

AndreasArvidsson linked an issue Dec 5, 2023 that may be closed by this pull request

New representation for scope handler tests #1524

Closed

3 tasks

pokey reviewed Dec 5, 2023

View reviewed changes

packages/common/src/scopeSupportFacets/getLanguageScopeSupport.ts Outdated Show resolved Hide resolved

Merge branch 'main' into scope_tests

7adcef6

pokey removed the to discuss Plan to discuss at meet-up label Dec 5, 2023

AndreasArvidsson added 2 commits December 5, 2023 18:32

cleanup

cfebfd9

rename

72ef3f5

pokey reviewed Dec 5, 2023

View reviewed changes

.pre-commit-config.yaml Outdated Show resolved Hide resolved

packages/common/src/scopeSupportFacets/textual.ts Outdated Show resolved Hide resolved

AndreasArvidsson and others added 3 commits December 6, 2023 02:20

Update .pre-commit-config.yaml

d427431

Co-authored-by: Pokey Rule <[email protected]>

fix

616988e

Remove unused type

2d8c499

pokey reviewed Dec 6, 2023

View reviewed changes

Remove examples and label

008f4ce

pokey approved these changes Dec 6, 2023

View reviewed changes

pokey enabled auto-merge December 6, 2023 17:29

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

Merged via the queue into main with commit 43cf0da Dec 6, 2023
13 checks passed

pokey deleted the scope_tests branch December 6, 2023 17:38

		@@ -0,0 +1,82 @@
		import { SimpleScopeTypeType } from "../types/command/PartialTargetDescriptor.types";

		const scopeSupportFacets = [

Scope tests #2053

Scope tests #2053

Conversation

AndreasArvidsson commented Nov 22, 2023 • edited Loading

Checklist

pokey commented Nov 22, 2023

AndreasArvidsson commented Nov 22, 2023 • edited Loading

pokey commented Nov 22, 2023 • edited Loading

AndreasArvidsson commented Nov 22, 2023 • edited Loading

AndreasArvidsson commented Nov 22, 2023

pokey left a comment • edited Loading

Choose a reason for hiding this comment

AndreasArvidsson commented Nov 30, 2023 • edited Loading

pokey commented Nov 30, 2023

AndreasArvidsson commented Nov 30, 2023

pokey commented Nov 30, 2023

AndreasArvidsson commented Dec 1, 2023

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

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

Choose a reason for hiding this comment

AndreasArvidsson Dec 1, 2023 • edited Loading

Choose a reason for hiding this comment

pokey Dec 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreasArvidsson Dec 2, 2023 • edited Loading

Choose a reason for hiding this comment

AndreasArvidsson commented Dec 5, 2023

pokey left a comment

Choose a reason for hiding this comment

pokey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreasArvidsson Dec 6, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreasArvidsson Dec 6, 2023 • 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

Choose a reason for hiding this comment

AndreasArvidsson Dec 6, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreasArvidsson commented Dec 6, 2023

AndreasArvidsson commented Nov 22, 2023 •

edited

Loading

AndreasArvidsson commented Nov 22, 2023 •

edited

Loading

pokey commented Nov 22, 2023 •

edited

Loading

AndreasArvidsson commented Nov 22, 2023 •

edited

Loading

pokey left a comment •

edited

Loading

AndreasArvidsson commented Nov 30, 2023 •

edited

Loading

AndreasArvidsson Dec 1, 2023 •

edited

Loading

pokey Dec 1, 2023 •

edited

Loading

AndreasArvidsson Dec 2, 2023 •

edited

Loading

AndreasArvidsson Dec 6, 2023 •

edited

Loading

AndreasArvidsson Dec 6, 2023 •

edited

Loading

AndreasArvidsson Dec 6, 2023 •

edited

Loading