Skip to content

"take round" also takes leading $ #1812

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

Open
jmegner opened this issue Aug 18, 2023 · 10 comments
Open

"take round" also takes leading $ #1812

jmegner opened this issue Aug 18, 2023 · 10 comments

Comments

@jmegner
Copy link
Collaborator

jmegner commented Aug 18, 2023

"take round air" on $($a) selects $($a), but it should select ($a).

@josharian josharian added the bug Something isn't working label Aug 18, 2023
@pokey
Copy link
Member

pokey commented Aug 19, 2023

That is intentional. We consider the $ to be part of the pair

@AndreasArvidsson
Copy link
Member

Defined here

To be honest I've never been one hundred percent sure about that decision.

@pokey
Copy link
Member

pokey commented Aug 19, 2023

The idea is to include the full delimiter token, similar to the way we handle f-strings and triple quotes in Python. But if @AndreasArvidsson @josharian and @jmegner all want to push for this one I'll cave

@jmegner
Copy link
Collaborator Author

jmegner commented Aug 19, 2023

....I now better appreciate the benefits of including the dollar sign, but I fear it's just going to cause more surprise and mistakes than benefit. Let me play with it more now that I know it is deliberate.

@auscompgeek auscompgeek removed the bug Something isn't working label Aug 19, 2023
@AndreasArvidsson AndreasArvidsson added the to discuss Plan to discuss at meet-up label Aug 20, 2023
@AndreasArvidsson
Copy link
Member

AndreasArvidsson commented Aug 24, 2023

After discussion with @pokey we came to the following conclusion

  • Matching pairs should be dumb and don't try to expand to anything else than the given pair
  • the string scope type should be implemented per language(eg f"foo" would be implemented by python) with a fall back to matching pairs. This is similar to what we do for item
  • The argument scope type could be used for interpolated variables like ${foo}

github-merge-queue bot pushed a commit that referenced this issue Sep 8, 2023
The fallback logic for determining whether to use text-based surrounding
pairs or parse-tree-based was broken for text fragment extractors based
on next-gen scope handlers (ie using the new `@textFragment` tag in a
query file).

This breakage meant that in a string like `"""hello"""` in Python, the
surrounding pair finder fell back to text-based, which resulted in it
thinking `"hello"` was a string, rather than `"""hello"""`, as would
happen if it fell back to parse-tree-based

This PR fixes the issue by unifying the fallback logic between legacy
and next-gen text fragment extractors, so that they don't fall out of
sync again

See also
#1812 (comment);
arguably that will make this PR irrelevant, but until then, it's better
to have string not be broken when we migrate a language to use
`@textFragment`

Note that the tests in this PR don't actually test the code yet, because
Python is still using legacy text fragment extractors. The tests will
start biting when this PR is merged into main and then merged into
#1862

- This PR is required by
#1862

## Checklist

- [ ] 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

---------

Co-authored-by: Andreas Arvidsson <andreas.arvidsson87@gmail.com>
@pokey
Copy link
Member

pokey commented Sep 9, 2023

the string scope type should be implemented per language(eg f"foo" would be implemented by python) with a fall back to matching pairs. This is similar to what we do for item

Note that one drawback of this approach is that if you have something like

aaa = "bbb 'ccc' ddd"

Saying "string cap" will give you the whole string "bbb 'ccc' ddd" rather than 'ccc'.

However, we have the same problem with "item", where we'll leap to the syntactic item even if you're inside a smaller textual item. We should be able to fix both of these issues more easily once item and pair are migrated to next-gen scopes, so I don't think this problem should keep us from implementing the proposal in @AndreasArvidsson's comment in the meantime

@AndreasArvidsson
Copy link
Member

AndreasArvidsson commented Sep 9, 2023

Yes if you just want to match on the ' delimiters you should use a matching pair and not the string scope type. Now we have just overloaded the same word to mean both of them, but in the future we would have this distinction.

@pokey
Copy link
Member

pokey commented Sep 10, 2023

Yes if you just want to match on the ' delimiters you should use a matching pair and not the string scope type. Now we have just overloaded the same word to mean both of them, but in the future we would have this distinction.

I don't think that lines up with what you propose in your bullet points above. See where it says "the string scope type should be implemented per language ... with a fall back to matching pairs"

@AndreasArvidsson
Copy link
Member

I see no contradicition in that? The string scope is defined per language and a string in a programming language is (mostly) well defined. Matching quotes inside a string you would have to use matching pairs for.

@pokey pokey removed the to discuss Plan to discuss at meet-up label Sep 14, 2023
@pokey pokey mentioned this issue Oct 3, 2023
13 tasks
@pokey
Copy link
Member

pokey commented Jun 12, 2024

I think the problem I propose in #1812 (comment) can be solved by a oneOf matcher combining matching pairs and syntactic string. Then it would just be nested as we'd like. We can do this once matching pairs are migrated to next-gen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants