Migrate python scope "value" to new scope handler #1877

saidelike · 2023-09-08T15:00:56Z

Checklist

I have executed the existing tests but they seem to fail for whatever reason. Not sure why (e.g. takeValue.yml from a quick glance
I have tested the visualize value command and all the take value from this file and they all work fine

If anyone has ideas why the unit tests don't work.

NOTES:

I haven't included the assignment symbols like ".", "=", "/=" as I don't think they are needed when using the tree-sitter but let me know if you think otherwise.
the "return" in the "return_statement" is something that was ignored too as not relevant anymore afaict

…arguments migrated to new scope handler

saidelike · 2023-09-08T15:02:34Z

And this is temporarily disabling the assignment values to see the domain of dictionary values:

pokey

Coming along! Missing

Domains on assignments
Leading delimiters on everything except return values (search in scm files for leading)

saidelike · 2023-09-08T15:56:45Z

Coming along! Missing

* [ ]  Domains on assignments

* [ ]  Leading delimiters on everything except return values (search in scm files for `leading`)

Ok domains on assignments done. Do you have examples for use cases where delimeters matter for the value scope? I understand the , in the arg scope case, but I don't see it for value...

pokey · 2023-09-08T19:16:01Z

It's if you say "chuck value". For example if you wanted to convert a dict to a set by deleting all the values, you could say "chuck every value" and it would clean up the :. Not totally clear how useful it is for assignments, but we support it today so I'd be inclined to preserve that behaviour. That's what the long list of assignment operators is doing in the original code (eg =, +=, etc)

Also, speaking of "every" you'll want to add iteration scopes as well

saidelike · 2023-09-10T06:40:46Z

Added value iteration scopes:

visualize value:

visualize value iteration:

… value'

pokey · 2023-09-10T08:14:54Z

queries/python.scm

It looks like you've lost a bunch of value captures by making the types more specific here. Search in tree-sitter-python for 'value'. Eg we lost values within match statement patterns, values in named expressions, etc. I'd be inclined to keep a generic value: (_) of some sort as that is often what we want

pokey

Ok I made some small tweaks in 094c70e. Note in particular how I added ! and !! marks to your comments and tweaked some of the example markers underneath those lines. See #1524 for more on that syntax

pokey · 2023-09-10T10:27:57Z

ok sorry thought you were sleeping so I force-pushed over my previous commit 😅

pokey · 2023-09-10T10:28:22Z

this PR is good to go now from my perspective; see 22bf6ea

pokey · 2023-09-10T10:31:04Z

all told, I made the following changes:

Added those ! and tweaked scope range markers as mentioned above
Removed extraneous parens from groupings where there was no predicate
Changed the way we do leading range to start from the end of previous named node instead of looking for =, +=, etc, as that simplified code and also fixed the failing tests that expected better whitespace cleanup
Added some tests to catch a few other value examples

pokey · 2023-09-10T10:31:56Z

The last issue remaining is that the new playground file has some invalid python syntax; see the complaining pre-commit step in CI

pokey · 2023-09-10T10:32:19Z

We're also not supporting iteration scope for multi-value return values, eg return a, b. That is definitely extra credit, though, so I'd be tempted to do that in follow-up PR

pokey · 2023-09-10T10:33:42Z

notice how the removal range extends all the way past the space before the = etc

pokey · 2023-09-10T10:35:41Z

data/playground/python/values.py

-    a = 0
-    b = 1
-    c = 2
+    pass


wow looks like we have some pretty aggressive "auto-formatting" going on here 😅. cc/ @auscompgeek any idea which formatter is doing this?

It's probably ruff's pyflakes (F) autofixes yeeting the unused variables, then the unused literal expressions?

We could either force playground to be compliant and avoid unused variables or maybe we could have exception for the playground data folder. I prefer the former as it forces us fixing typos and make sure it is valid language since otherwise the cursorless commands won't work in some cases due to typos

I guess I would prefer a blanket exception for everything in the playground. I have several things in the playground that are not gofmt compliant (but are valid go code ) specifically so that I can experiment to see whether cursorless does the right thing with them.

I am very much on the fence, but lean slightly towards formatting playground but being liberal with comments to disable auto-formatter / linter where we want. @josharian is it not easy to disable gofmt on a per-line basis?

I don't feel strongly, tho. I could see the benefit to having full freedom in the playground

We could have the pre-commit step check for the actual property we want, namely valid syntax

It depends in part on how active the formatter is. There is some danger of writing queries that only match when using the same formatter as us, particularly if the formatter is not whitespace only. 🤷🏽

Yeah that's a good point. I think maximal flexibility is not necessarily a bad thing in the playground. Interesting idea to just check for valid syntax. Altho tbh we even might want to violate that in some cases because error tolerance is one of the reasons we like tree-sitter

I'm convinced. See 43939f7. Any objections @auscompgeek @josharian @AndreasArvidsson ?

No sounds resonable

pokey · 2023-09-10T10:46:26Z

queries/python.scm

+  (_) @_.leading.start.endOf
+  .
+  right: (_) @value @_.leading.end.startOf
+) @_.domain


fwiw I do think this pattern and the following one (augmented_assignment) are potentially a compelling use case for #has-type? predicate, as that would allow them to be merged into one pattern. @AndreasArvidsson @josharian wdyt?

To me it is the same as dictionary vs dictionary_comprehension as discussed in 36c2f4c

Agreed it is quite similar. In that case we managed to avoid duplication by rewriting the pattern another way, but I don't see a way out of it here other than has-type

pokey · 2023-09-10T10:48:49Z

queries/python.scm

+;; NOTE: in tree-sitter, both "return" and the "1" are children of `return_statement`
+;; but "return" is anonymous whereas "1" is named node, so no need to exclude explicitly


This comment might be overkill. We leverage this kind of assumption all over the place. For example, I'm leveraging this kind of assumption in my leading matchers above, where I rely on the fact that = is anonymous whereas the name / type before it is not

saidelike · 2023-09-10T11:23:24Z

notice how the removal range extends all the way past the space before the = etc

Looks great. What command can you say to see the removal range?

pokey · 2023-09-10T11:32:00Z

"visualize value removal"

see docs for visualizer, although at this point I think you now know everything in that doc 😄

pokey · 2023-09-10T11:51:54Z

The last issue remaining is that the new playground file has some invalid python syntax; see the complaining pre-commit step in CI

Once this is fixed I think we're good to merge

pokey

Looks good to me!

Cedric Halbronn added 3 commits September 8, 2023 15:47

value for assignment statements, dictionaries, return statements and …

9df53f8

…arguments migrated to new scope handler

add Python value scope data playground

5a736b9

add test from takeValue.yml

af74a74

saidelike requested a review from pokey as a code owner September 8, 2023 15:00

pokey reviewed Sep 8, 2023

View reviewed changes

domains on value assignments

1305812

auscompgeek added lang-python Issues related to Python programming language support scope-migration Migrating scopes to next-gen scope implementation labels Sep 9, 2023

Cedric Halbronn added 2 commits September 9, 2023 16:17

leading characters for Python values

3fa6e08

Python value iteration scopes

e354ab1

fix handling of arguments with both : and = in them when using 'chunk…

d963952

… value'

pokey reviewed Sep 10, 2023

View reviewed changes

pokey force-pushed the migrate-python-scope-value branch from 5a93b81 to 094c70e Compare September 10, 2023 10:10

pokey reviewed Sep 10, 2023

View reviewed changes

Fixes

22bf6ea

pokey force-pushed the migrate-python-scope-value branch from 094c70e to 22bf6ea Compare September 10, 2023 10:26

Merge branch 'main' into migrate-python-scope-value

fa803b5

[pre-commit.ci lite] apply automatic fixes

092fd72

pokey reviewed Sep 10, 2023

View reviewed changes

tweak comments

bec9772

pokey reviewed Sep 10, 2023

View reviewed changes

pokey added 2 commits September 10, 2023 15:42

Don't apply pre-commit to playground

43939f7

switch playground to CRLF

53debdf

pokey approved these changes Sep 10, 2023

View reviewed changes

pokey enabled auto-merge September 10, 2023 14:48

pokey added this pull request to the merge queue Sep 10, 2023

Merged via the queue into cursorless-dev:main with commit 065f98a Sep 10, 2023

auscompgeek mentioned this pull request Sep 23, 2023

Black is not formatting anything #1909

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate python scope "value" to new scope handler #1877

Migrate python scope "value" to new scope handler #1877

saidelike commented Sep 8, 2023

saidelike commented Sep 8, 2023

pokey left a comment

saidelike commented Sep 8, 2023

pokey commented Sep 8, 2023 •

edited

Loading

saidelike commented Sep 10, 2023

pokey Sep 10, 2023

pokey left a comment

pokey commented Sep 10, 2023

pokey commented Sep 10, 2023

pokey commented Sep 10, 2023

pokey commented Sep 10, 2023

pokey commented Sep 10, 2023

pokey commented Sep 10, 2023 •

edited

Loading

pokey Sep 10, 2023

auscompgeek Sep 10, 2023

saidelike Sep 10, 2023

josharian Sep 10, 2023

pokey Sep 10, 2023

josharian Sep 10, 2023

josharian Sep 10, 2023

pokey Sep 10, 2023

pokey Sep 10, 2023

AndreasArvidsson Sep 10, 2023

pokey Sep 10, 2023 •

edited

Loading

saidelike Sep 10, 2023

pokey Sep 10, 2023 •

edited

Loading

pokey Sep 10, 2023

saidelike commented Sep 10, 2023

pokey commented Sep 10, 2023

pokey commented Sep 10, 2023

pokey left a comment

		;; NOTE: in tree-sitter, both "return" and the "1" are children of `return_statement`
		;; but "return" is anonymous whereas "1" is named node, so no need to exclude explicitly

Migrate python scope "value" to new scope handler #1877

Migrate python scope "value" to new scope handler #1877

Conversation

saidelike commented Sep 8, 2023

Checklist

saidelike commented Sep 8, 2023

pokey left a comment

Choose a reason for hiding this comment

saidelike commented Sep 8, 2023

pokey commented Sep 8, 2023 • edited Loading

saidelike commented Sep 10, 2023

Choose a reason for hiding this comment

pokey left a comment

Choose a reason for hiding this comment

pokey commented Sep 10, 2023

pokey commented Sep 10, 2023

pokey commented Sep 10, 2023

pokey commented Sep 10, 2023

pokey commented Sep 10, 2023

pokey commented Sep 10, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pokey Sep 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pokey Sep 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saidelike commented Sep 10, 2023

pokey commented Sep 10, 2023

pokey commented Sep 10, 2023

pokey left a comment

Choose a reason for hiding this comment

pokey commented Sep 8, 2023 •

edited

Loading

pokey commented Sep 10, 2023 •

edited

Loading

pokey Sep 10, 2023 •

edited

Loading

pokey Sep 10, 2023 •

edited

Loading