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(treesitter)!: use new standard captures #27067

Merged
merged 1 commit into from
Jan 21, 2024
Merged

Conversation

clason
Copy link
Member

@clason clason commented Jan 17, 2024

Problem: Sharing queries with upstream tree-sitter and Helix is difficult. Also, queries look funky.

Solution: Use treesitter standard captures and Helix (Sublime-style) captures where reasonable. Document them to make them official. Also, format queries with formatting tool. Sync queries with nvim-treesitter.

This essentially defines a full set of canonical capture names and is a step toward declaring treesitter highlighting as stable. (It also effectively transfers ownership of the "canonical list" to Neovim core.)

See nvim-treesitter/nvim-treesitter#5831

@echasnovski I defined all treesitter highlight groups from the "standard set" by linking to default captures (unless the standard fallback looked reasonable); I expect you'll fine tune this based on the default colorscheme. Nevertheless, if you notice a group where a different link would be better, let me know.

src/nvim/highlight_group.c Outdated Show resolved Hide resolved
src/nvim/highlight_group.c Show resolved Hide resolved
src/nvim/highlight_group.c Outdated Show resolved Hide resolved
src/nvim/highlight_group.c Outdated Show resolved Hide resolved
Comment on lines +318 to +317
"default link @markup.list Special",
"default link @markup.list.checked DiagnosticOk",
"default link @markup.list.unchecked DiagnosticWarn",
Copy link
Member Author

Choose a reason for hiding this comment

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

Note

@gpanders
Copy link
Member

Changing capture names will break user defined queries so this should be marked as a breaking change.

@clason

This comment was marked as resolved.

@gpanders

This comment was marked as resolved.

@clason

This comment was marked as resolved.

@clason clason changed the title feat(treesitter): use new standard captures feat(treesitter)!: use new standard captures Jan 17, 2024
@wookayin wookayin added this to the 0.10 milestone Jan 17, 2024
Copy link
Member

@echasnovski echasnovski left a comment

Choose a reason for hiding this comment

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

This is a monumental improvement in terms of usability, I should say. Seeing updated documentation with actual descriptions of what group is supposed to do warms my color scheme author heart.

Sorry for a lot of comments. None of them is blocking (aside from @module, I'd say) and can be resolved in my (planned) follow up for default color scheme.

src/nvim/highlight_group.c Outdated Show resolved Hide resolved
src/nvim/highlight_group.c Show resolved Hide resolved
src/nvim/highlight_group.c Show resolved Hide resolved
src/nvim/highlight_group.c Outdated Show resolved Hide resolved
src/nvim/highlight_group.c Outdated Show resolved Hide resolved
src/nvim/highlight_group.c Outdated Show resolved Hide resolved
src/nvim/highlight_group.c Outdated Show resolved Hide resolved
src/nvim/highlight_group.c Show resolved Hide resolved
src/nvim/highlight_group.c Outdated Show resolved Hide resolved
src/nvim/highlight_group.c Outdated Show resolved Hide resolved
@lucario387

This comment was marked as resolved.

@echasnovski

This comment was marked as resolved.

@lucario387

This comment was marked as resolved.

@echasnovski

This comment was marked as resolved.

@clason clason force-pushed the ts-captures branch 2 times, most recently from 68a8ceb to 9f62edb Compare January 18, 2024 15:05
@clason clason marked this pull request as ready for review January 19, 2024 16:31
@clason
Copy link
Member Author

clason commented Jan 19, 2024

These changes are now live on nvim-treesitter, so marking as ready for review.

runtime/doc/news.txt Show resolved Hide resolved
@diff.minus deleted text (for diff files)
@diff.delta changed text (for diff files)

@tag XML tag names
Copy link
Member

Choose a reason for hiding this comment

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

IDK how clear we were about this before, but what this makes me think as a vanilla Neovim user that has no advanced context/knowledge of these changes is: "Ok I don't need to worry about @tag in my color theme bc I don't use XML at all", yet a seemingly unrelated language like yuck uses it in its queries.

Maybe it would be worth it to document such a gotcha here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't that be on yuck query maintainers to document that :? 🤔 I tried a

(button :onclick "notify-send Left"
  :onrightclick "notify-send Right"
  "Click Me!") ;; this is a label nested within a button

Where button is the symbol, and the syntax have some similarities to XML

Copy link
Member

Choose a reason for hiding this comment

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

Fair point, but I'm bringing this up from the perspective of a Neovim user that's updating their TS queries to satisfy their colorful desires: If I find a query that at first glance look language/file extension exclusive, I might forget to consider which other languages might be affected.

I highlight these capture groups in particular because XML is included in the name, instead of an example as in previous groups.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's fine, feel free to suggest a better description. (I don't do weebdev.)

Copy link
Contributor

Choose a reason for hiding this comment

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

We can clean up improperly used @tag highlights in nvim-treesitter later (probably should have handled it in my consistency PR tbh), but yuck (and other?) widgets are similar to XML tags and can maybe also use it.

Update: nvim-treesitter/nvim-treesitter#5942

runtime/queries/bash/highlights.scm Show resolved Hide resolved
Problem: Sharing queries with upstream and Helix is difficult due to
different capture names.

Solution: Define and document a new set of standard captures that
matches tree-sitter "standard captures" (where defined) and is closer to
Helix' Atom-style nested groups.

This is a breaking change for colorschemes that defined highlights based
on the old captures. On the other hand, the default colorscheme now
defines links for all standard captures (not just those used in bundled
queries), improving the out-of-the-box experience.
@clason clason marked this pull request as ready for review January 20, 2024 17:19
@clason clason merged commit f5dc453 into neovim:master Jan 21, 2024
29 checks passed
@clason clason deleted the ts-captures branch January 21, 2024 09:41
Comment on lines +307 to +310
"default link @markup.raw Comment",
"default link @markup.quote Comment",
"default link @markup.math Comment",
"default link @markup.environment Comment",
Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, not sure I agree with this anymore -- I inherited from the old links, but it seems wrong. Special seems much better (and making the other three derive from raw).

@echasnovski ?

Copy link
Member

Choose a reason for hiding this comment

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

If this is about only @markup.environment, then I don't have any particular opinion on this one. Judging purely by the meaning, I'd say that it should link to @module.
Making other link to @markup.raw is reasonable.

Copy link
Member Author

Choose a reason for hiding this comment

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

...and @module links to Structure ;)

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

Successfully merging this pull request may close these issues.

None yet

10 participants