Skip to content
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

Add language overrides for delimiter maps #2012

Merged
merged 3 commits into from
Jun 13, 2024

Conversation

fidgetingbits
Copy link
Collaborator

@fidgetingbits fidgetingbits commented Nov 9, 2023

This is me breaking out some functionality that was part of PR #1911 since that PR has lots of work to be done and it may still benefit other languages that get in sooner. I've applied the suggested changes from pokey in that PR's code review.

I've also added a placeholder map for lua which uses [[ ]] for multiline support as noted in PR #1962, however I realized as I went to add it that it's not as simple as the way I did it for nix because I can't just reuse the existing singleQuote entry since lua actually uses single quotes. So this may actually be a bit harder, and reminds me of this discussion. So before I go randomly hacking stuff I'm curious what you think the best approach here is.

These are the comments I had left in the Nix PR about these changes:

  • I called returning delimiterToText as getSimpleDelimiterMap to kind of mirror complexDelimiterMap.

  • I'm not sure is if you still want a delimiterMap.ts standalone file, and then a getSimpleDelimiterMap.ts only for that function. Now delimiterToText isn't referenced anywhere else, so seemed maybe okay to keep them together.

  • I also thought about adding getComplexDelimiterMap(), but atm because complexDelimiterMap only ever uses the keys from the delimiterToText it won't change even if the language has different values, so seemed unnecessary. These are all things I could guess at your preferences, but may as well ask instead.

  • I also noticed leftToRightMap in that file isn't actually used anywhere so can be deleted I think, but also not sure about doing that as part of a totally unrelated code change, to keep commits clean.

Checklist

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

@fidgetingbits fidgetingbits marked this pull request as draft November 9, 2023 02:17
@fidgetingbits fidgetingbits mentioned this pull request Nov 9, 2023
10 tasks
@pokey
Copy link
Member

pokey commented Nov 9, 2023

I would be tempted to introduce a new delimiter type for [[, and then make complex delimiter list customisable so you can include it

WDYT @AndreasArvidsson

@fidgetingbits fidgetingbits mentioned this pull request Nov 13, 2023
13 tasks
@fidgetingbits
Copy link
Collaborator Author

See #1911 (comment) for a comment about some code from this PR.

@AndreasArvidsson
Copy link
Member

I would be tempted to introduce a new delimiter type for [[, and then make complex delimiter list customisable so you can include it

WDYT @AndreasArvidsson

I think I need a bit more info on your suggestion

@pokey
Copy link
Member

pokey commented Nov 13, 2023

I would be tempted to introduce a new delimiter type for [[, and then make complex delimiter list customisable so you can include it
WDYT @AndreasArvidsson

I think I need a bit more info on your suggestion

the problem is that in lua, [[ and ]] can be used as delimiters for a quote. So I'm proposing to add those as a new delimiter type doubleSquareBrackets. Then we'd need "string" to include those. "string" is defined as a complex delimiter string: ["singleQuotes", "doubleQuotes", "backtickQuotes"],, so we'd add doubleSquareBrackets to that list, but only for Lua.

One thing we'd need to think about is that that would cause [[foo]] to be viewed as a pair for our text-based matchers, and it wouldn't be considered a "square", which is probably not great. So we'd prob need a way for that pair not to be active outside of Lua

@AndreasArvidsson
Copy link
Member

I would be tempted to introduce a new delimiter type for [[, and then make complex delimiter list customisable so you can include it
WDYT @AndreasArvidsson

I think I need a bit more info on your suggestion

the problem is that in lua, [[ and ]] can be used as delimiters for a quote. So I'm proposing to add those as a new delimiter type doubleSquareBrackets. Then we'd need "string" to include those. "string" is defined as a complex delimiter string: ["singleQuotes", "doubleQuotes", "backtickQuotes"],, so we'd add doubleSquareBrackets to that list, but only for Lua.

One thing we'd need to think about is that that would cause [[foo]] to be viewed as a pair for our text-based matchers, and it wouldn't be considered a "square", which is probably not great. So we'd prob need a way for that pair not to be active outside of Lua

I agree, but we still need to make square actually work of course.

@pokey
Copy link
Member

pokey commented Nov 14, 2023

Ok @fidgetingbits I think we're in agreement. Lmk if you need help figuring out how to put the above to practice

@fidgetingbits
Copy link
Collaborator Author

fidgetingbits commented Dec 15, 2023

Ok @fidgetingbits I think we're in agreement. Lmk if you need help figuring out how to put the above to practice

Ya some guidance would be good, as much as I'd love to try to figure it out on my own. My typescript experience consists of what's in this PR. I suspect it would take me quite a long time to work out what to do.

@pokey
Copy link
Member

pokey commented Dec 15, 2023

Ok I'll take this over when I get a minute

@pokey pokey self-assigned this Dec 15, 2023
@pokey pokey force-pushed the per-language-pair-delimiters branch from af665ed to 0c4dd37 Compare June 12, 2024 20:23
@pokey pokey force-pushed the per-language-pair-delimiters branch from 0c4dd37 to e82832f Compare June 12, 2024 20:25
Copy link
Member

@pokey pokey left a comment

Choose a reason for hiding this comment

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

Ok @fidgetingbits I made some tweaks. Given some more recent discussions (#1812 (comment)), I don't think this is the long-term solution, but:

  • It's not too much code,
  • It's not too bad an abstraction, and
  • I need it for a tree-sitter upgrade I'm doing to handle strings in Python

Let me know if you're happy with my changes (see e82832f)

Also, @AndreasArvidsson would be good to get a second 👀 as I've made some changes here and we've discussed syntactic vs text pairs a lot

@pokey pokey marked this pull request as ready for review June 12, 2024 20:27
@AndreasArvidsson
Copy link
Member

Looks reasonable to me

@fidgetingbits
Copy link
Collaborator Author

Ok @fidgetingbits I made some tweaks. Given some more recent discussions (#1812 (comment)), I don't think this is the long-term solution, but:

* It's not too much code,

* It's not too bad an abstraction, and

* I need it for a tree-sitter upgrade I'm doing to handle strings in Python

Let me know if you're happy with my changes (see e82832f)

Added lua test looks good to me. Your changes look good too, reads cleaner.

@pokey pokey added this pull request to the merge queue Jun 13, 2024
Merged via the queue into cursorless-dev:main with commit 1abf38d Jun 13, 2024
15 checks passed
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

Successfully merging this pull request may close these issues.

3 participants