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

Improved tree-sitter queries #896

Merged
merged 8 commits into from
Nov 3, 2021
Merged

Improved tree-sitter queries #896

merged 8 commits into from
Nov 3, 2021

Conversation

kirawi
Copy link
Member

@kirawi kirawi commented Oct 23, 2021

@variable.property -> @variable.other.member
@property -> @variable.other.member
@number -> @constant.numeric.{integer, float}
@escape -> @constant.character.escape
@constant.character is now uniformly used for all chars.

(let_declaration
pattern: [
((identifier) @variable)
((tuple_pattern
Copy link
Member Author

@kirawi kirawi Oct 23, 2021

Choose a reason for hiding this comment

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

Should this be pulled out to match all tuple_patterns? I don't know if there are any other cases where it's used that we probably don't want to highlight it as a variable. I'll do it later once I refactor highlights.scm to completely cover all highlighting based on tree-sitter-rust/grammar.js

Copy link
Member

Choose a reason for hiding this comment

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

Not sure, we could do that and simplify

@archseer
Copy link
Member

variable.field: What you really want is variable.other.member as declared by Sublime Text:

Fields, properties, members and attributes of a class or other data structure should use:

Here's an example of atom's tree-sitter Rust grammar using it: https://github.com/atom/atom/blob/fd5577910528774b549e94407b555da89f55db79/packages/language-rust-bundled/grammars/tree-sitter-rust.cson#L39

@kirawi
Copy link
Member Author

kirawi commented Oct 23, 2021

I feel that people wouldn't understand what that means just by looking at it, versus variable.field.

@archseer
Copy link
Member

A struct member is pretty common terminology. I'd rather stay in line with TextMate/Sublime scopes since that's what most grammars already use (including VSCode)

@archseer
Copy link
Member

archseer commented Oct 23, 2021

I'm also looking at other grammars to see if type.enum.variant is a good choice or not for enums, doesn't look like Atom styles it. VSCode probably has a more detailed grammar

variable.property -> variable.field
property -> variable.field
@kirawi
Copy link
Member Author

kirawi commented Oct 23, 2021

Looks like variable.other.enumMember

@archseer
Copy link
Member

You can use a scope inspector to see the scopes used, but I don't have VSCode installed: https://code.visualstudio.com/api/language-extensions/syntax-highlight-guide#textmate-tokens-and-scopes

@kirawi
Copy link
Member Author

kirawi commented Oct 23, 2021

Capture

@archseer
Copy link
Member

archseer commented Oct 23, 2021

Hmm microsoft/vscode#126957

I also found this: https://code.visualstudio.com/api/language-extensions/semantic-highlight-guide#predefined-textmate-scope-mappings

Based on your screenshot: Looks like the "textmate scopes" is simply type.rust, I think the different styling you previously noticed for enum variants (outside of definitions) could be due to special casing Result/Option?

I mean in cases like MyEnum::Hello(1), outside of pub enum ... { ... }

@kirawi
Copy link
Member Author

kirawi commented Oct 23, 2021

They're both styled as variable.other.enummember. I think you're referring to struct constructors? Though actually it seems like those are being highlighted as entity.name.type.struct.rust

@archseer
Copy link
Member

Yeah that's what I meant, it looks like definitions have different scopes from constructors/uses in code. Similar to @function vs @variable.function.

Since we're already changing a couple scopes, let's also do:

  • @character -> @constant.character
  • @escape -> @constant.character.escape
  • @number/@float to @constant.numeric.{integer, float, complex}.

That way we reduce the amount of top level scopes, making it easier to style in simple themes (you just need to define @constant, or @constant.numeric to style all numbers)

@kirawi
Copy link
Member Author

kirawi commented Oct 28, 2021

Just need to update the book.

@archseer
Copy link
Member

All the themes need to be updated to use these new scopes too.

updated book and themes to reflect scope changes
@kirawi
Copy link
Member Author

kirawi commented Oct 30, 2021

@raygervais I noticed that

"ui.popup" = { bg = "base01" }
"ui.window" = { bg = "base00" }
"ui.help" = { bg = "base01", fg = "base06" }

have duplicated keys in base16.

@kirawi
Copy link
Member Author

kirawi commented Oct 30, 2021

How do I fix the submodule change? I don't know how that even happened.

@sudormrfbin
Copy link
Member

How do I fix the submodule change? I don't know how that even happened.

Is it the change with elixir ? #875

@kirawi
Copy link
Member Author

kirawi commented Oct 30, 2021

It's with cpp, presumably this happened when I did git pull upstream master

@archseer
Copy link
Member

git sync; git submodule update --init?

@kirawi
Copy link
Member Author

kirawi commented Oct 31, 2021

Doesn't seem like it. Maybe my case is different since I set upstream to this repo while origin points to my fork?

@archseer
Copy link
Member

Well now that you committed and pushed the submodule update it considers that to be what it's syncing to maybe?

@archseer
Copy link
Member

Ah sorry, was supposed to be git submodule sync too. Easiest fix would be to manually update the submodule to the same commit as in the repo

@kirawi
Copy link
Member Author

kirawi commented Oct 31, 2021

How do you do that?

@kirawi
Copy link
Member Author

kirawi commented Oct 31, 2021

Curse you git... curse you.

(false)
] @constant.builtin.boolean
(null) @constant.builtin
(number) @constant.numeric
Copy link
Member

Choose a reason for hiding this comment

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

Maybe constant.numeric.integer? I know it's not necessarily correct, but if someone defines constant.numeric.integer and constant.numeric.float but not constant.numeric then this won't get highlighted.

Copy link
Member

Choose a reason for hiding this comment

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

Though I guess we should just recommend styling constant.numeric

@kirawi kirawi changed the title Updated theme scopes and improved Rust highlights.scm Improved tree-sitter queries Nov 2, 2021
@archseer archseer merged commit ee889aa into helix-editor:master Nov 3, 2021
@kirawi kirawi deleted the scm branch November 3, 2021 03:01
archseer added a commit that referenced this pull request Nov 8, 2021
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

3 participants