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

feat: reintroduce raw-strings #117

Merged
merged 61 commits into from
Nov 22, 2024
Merged

feat: reintroduce raw-strings #117

merged 61 commits into from
Nov 22, 2024

Conversation

mrdgo
Copy link
Contributor

@mrdgo mrdgo commented Sep 25, 2024

Discussion with tree-sitter devs

This will break the current nvim-treesitter config. They have to update to also use scanner.c. People using that config might want to use "our" way to install ts-nu.

After merge, it might be necessary to run :TSUpdate nu at least once. I don't know about treesitter's caching, so let's just hope that nobody's setup breaks.

With that scanner, we can also create custom logic for unquoted strings.

@mrdgo
Copy link
Contributor Author

mrdgo commented Nov 21, 2024

Incorporated all reviews, removed the pipeline changes that neither provide insights nor prevent anything. Maybe pipeline is a separate issue that should be addressed at some point.

@fdncred
Copy link
Collaborator

fdncred commented Nov 21, 2024

Ping me if/when we're ready to land this and I'll push the button.

@blindFS
Copy link
Contributor

blindFS commented Nov 21, 2024

@fdncred I'm cool with it now, kudos to @mrdgo !

@mrdgo mrdgo mentioned this pull request Nov 21, 2024
@mrdgo
Copy link
Contributor Author

mrdgo commented Nov 21, 2024

Thanks to @blindFS for the thorough review! Is there someone else who would want to review? Otherwise, I'd be ready to land, again

@mrdgo mrdgo changed the title feat: reintroduce raw-strings Draft:feat: reintroduce raw-strings Nov 21, 2024
@mrdgo
Copy link
Contributor Author

mrdgo commented Nov 21, 2024

Aaah, wait a second

Edit: okay, now!

@mrdgo mrdgo marked this pull request as draft November 21, 2024 15:19
@mrdgo mrdgo changed the title Draft:feat: reintroduce raw-strings feat: reintroduce raw-strings Nov 21, 2024
@mrdgo mrdgo marked this pull request as ready for review November 21, 2024 15:21
src/scanner.c Outdated Show resolved Hide resolved
src/scanner.c Outdated Show resolved Hide resolved
queries/nu/highlights.scm Outdated Show resolved Hide resolved
val_string is a parent of raw_string_x and already highlighted
@fdncred
Copy link
Collaborator

fdncred commented Nov 22, 2024

looks like we're closer, just have a conflict now.

@fdncred
Copy link
Collaborator

fdncred commented Nov 22, 2024

Are we ready?

@blindFS
Copy link
Contributor

blindFS commented Nov 22, 2024

Are we ready?

I think so.

@fdncred fdncred merged commit 9eedcb3 into nushell:main Nov 22, 2024
3 checks passed
@mrdgo
Copy link
Contributor Author

mrdgo commented Nov 22, 2024

@clason does nvim-ts' lockfile interfere, even when I override the install_info?

@clason
Copy link

clason commented Nov 22, 2024

Yes, the lockfile takes precedence. (Nvim-ts is not a general purpose installer; you can shoehorn additional parsers but it's not designed for replacing tracked parsers.)

@mrdgo
Copy link
Contributor Author

mrdgo commented Nov 22, 2024

Yes, the lockfile takes precedence. (Nvim-ts is not a general purpose installer; you can shoehorn additional parsers but it's not designed for replacing tracked parsers.)

Would you then pretty-please be so kind 😇

@clason
Copy link

clason commented Nov 22, 2024

We have automation for that

(Or PR welcome, if queries need to be adapted.)

fdncred pushed a commit that referenced this pull request Nov 28, 2024
This PR fixes #157 .
The problem is rooted in my [review
suggestion](#117 (comment)),
so the change is reverted.

But I don't get why, how is whitespaces automatically skipped by
`extras` for `$._str_double_quotes`, but not `$._raw_string`.
@mrdgo do you have any idea?

Edit: I get it wrong, the scanner is handled earlier than `extras` so it
may override the behavior of the later.
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.

5 participants