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(grammar): Expose wildcard imports as named nodes #130

Merged
merged 1 commit into from
Jul 29, 2024

Conversation

ValdezFOmar
Copy link
Contributor

Parse "." and "*" for wildcard imports as separate nodes, allowing them to be more precisely highlighted.
Create a new named node wildcard_import.

Parse "." and "*" for wildcard imports as separate nodes, allowing them
to be more precisely highlighted.
Create a new named node `wildcard_import`.
@ValdezFOmar
Copy link
Contributor Author

I've only notice now that a similar pull request was opened (#122). But something different is that my implementation exposes . and * as separate nodes instead of being only one, which will allow more precise highlighting.
The use case for this is in nvim-treesitter (nvim-treesitter/nvim-treesitter#6979) where we are trying to highlight all wildcard imports (* specifically) the same. Please consider merging my PR instead 🙏

@@ -167,11 +167,13 @@ module.exports = grammar({

import_header: $ => seq(
"import",
$.identifier,
optional(choice(seq(".*"), $.import_alias)),
Copy link

Choose a reason for hiding this comment

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

Note that this construct (single child seq rule) is currently broken on tree-sitter master, so this (or a similar) change is needed to keep being able to generate a compatible parser.

@clason
Copy link

clason commented Jul 28, 2024

@fwcd @VladimirMakaev what's the current maintenance situation with this parser? I see a number of (even necessary) maintenance PRs being left unmerged, and I worry that sooner or later this parser will become unusable for downstream like Neovim.

@fwcd
Copy link
Owner

fwcd commented Jul 29, 2024

what's the current maintenance situation with this parser?

It's maintained, but not super actively (at least on my part), largely due to a lack of spare time and other important projects. PRs are always welcome though, smaller ones are more likely to be merged quickly.

@fwcd
Copy link
Owner

fwcd commented Jul 29, 2024

Please regenerate the parser on an x86_64 Linux machine: #92 The simplest way to do that is in CI using the "Regenerate Parser" action, which should be available in your fork too (if not, leave a note). Docker also works.

@clason
Copy link

clason commented Jul 29, 2024

That is not the issue; the problem is that the CI is using a different (outdated?) tree-sitter version. (Regenerating with current tree-sitter versions -- which do make a difference, especially in recent versions -- is exactly part of the parser maintenance I was referring to above.)

@fwcd
Copy link
Owner

fwcd commented Jul 29, 2024

The CI should use the same version of tree-sitter as the package, that is 0.22.1, and I would strongly recommend using the npm-installed version of tree-sitter for local development too. We pin it, because the playground and other tools have to be kept in sync, otherwise it becomes hard to keep track of the different environments that everyone builds the grammar in.

Updating to 0.22.6 is planned, but, as mentioned above, we have to be a bit careful about not breaking e.g. the playground by just bumping the version:

@ObserverOfTime
Copy link
Contributor

It's maintained, but not super actively (at least on my part), largely due to a lack of spare time and other important projects.

If you need help with maintenance, we'd be happy to adopt it under tree-sitter-grammars.

@fwcd
Copy link
Owner

fwcd commented Jul 29, 2024

That's very nice, thanks, but I still prefer to keep it here for now. I originally put a lot of work into writing this grammar and also maintain the packages under my name, so I'd like to keep the canonical version here for now. Feel free to fork it into your organization if that suits your workflow!

@ObserverOfTime
Copy link
Contributor

ObserverOfTime commented Jul 29, 2024

I mean you will keep maintaining it and the packages just as before, but have more hands on deck.

In any case, I'm personally fine with helping out whether it's here or there.

@fwcd fwcd merged commit 9cd6438 into fwcd:main Jul 29, 2024
1 of 2 checks passed
@fwcd
Copy link
Owner

fwcd commented Jul 29, 2024

Thanks!

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

Successfully merging this pull request may close these issues.

4 participants