nushell / tree-sitter-nu

A tree-sitter grammar for nu-lang, the language of nushell
MIT License
121 stars 27 forks source link

feat: reintroduce raw-strings #117

Closed mrdgo closed 3 days ago

mrdgo commented 2 months ago

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 commented 5 days ago

@blindFS Feel free to try this yourself! The code stands, it passes tests and correctly highlights in neovim-nightly. Please follow the link in the description (discussion with tree-sitter-devs) and see the current show-stopper. As soon as I start the exact same setup within neovim 0.10.2/0.10.1/0.9.5, it crashes without any error message. The only thing that I found out is a null-pointer dereference - which doesn't make sense, because the scanner works standalone and in neovim-nightly. I am deeply puzzled and don't really have an idea how to procceed

Edit: make sure to change the config in plugin/init.lua:

require("nvim-treesitter.parsers").get_parser_configs().nu = {
    install_info = {
        url = "~/git/tree-sitter-nu", -- path to your checkout
        files = { "src/parser.c", "src/scanner.c" },
        branch = "raw-strings-pt2",
    },
    filetype = "nu",
}
siph commented 5 days ago

fwiw this is working for me (3c50ee4186153dd768141c9d0a2fcc1fdcd47b03) in v0.10.2: Screenshot_20241120_144941

Screenshot_20241120_145028

I'm using nix to build the grammar and neovim.

blindFS commented 5 days ago

I also have no problem with your changes made on-top of current main branch:

image image

My neovim is installed from nix-darwin for macos.

clason commented 4 days ago

And in general, I would strongly recommend to make conceptually unrelated changes in separate PRs; that makes it much easier to review and merge preliminary steps.

(You can cherry-pick commits from PRs that are required to be merged first, as long as you clearly label them as such and rebase.)

mrdgo commented 4 days ago

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 commented 4 days ago

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

blindFS commented 4 days ago

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

mrdgo commented 4 days ago

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 commented 4 days ago

Aaah, wait a second

Edit: okay, now!

fdncred commented 3 days ago

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

fdncred commented 3 days ago

Are we ready?

blindFS commented 3 days ago

Are we ready?

I think so.

mrdgo commented 3 days ago

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

clason commented 3 days ago

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 commented 3 days ago

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 :innocent:

clason commented 3 days ago

We have automation for that

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