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

prefer elixir-lang/tree-sitter-elixir #830

Merged

Conversation

the-mikedavis
Copy link
Member

@IceDragon200's fork is based on ananthakumaran/tree-sitter-elixir which was recently deprecated in favor of the new implementation by the Elixir core team: elixir-lang/tree-sitter-elixir.

There isn't a huge difference in the results but there are some niceties such as @doc/@typedoc/@moduledoc being highlighted similar to a comment (since it's treated as documentation instead of a value), and some better handling of constants like charaters and numbers.

here's a side-by-side comparison...

helix-mint-websocket-compare

current highlighting on the left, this PR's changes on the right

If there are any special cases you want me to compare and screen grab, let me know!


I'll keep this as a draft for about a week or so as I test out the new parser and try to make it break/crash/panic.

@IceDragon200
Copy link
Contributor

Hey, I'm all in favour of ditching my fork for an official implementation, you have my blessing when your PR is ready

@archseer
Copy link
Member

Nice work! This looks great.

@kirawi
Copy link
Member

kirawi commented Oct 12, 2021

Could you also add the new scope captures to the book?

@the-mikedavis
Copy link
Member Author

Can do 👍

I missed that doc on my first pass but I'll give it a look and see if I can rewrite any captures to use those, then add any that don't have a place.

@the-mikedavis the-mikedavis force-pushed the official-elixir-tree-sitter branch from 66e4018 to 38c1585 Compare October 12, 2021 21:40
@@ -119,6 +119,7 @@ We use a similar set of scopes as
- `special`
- `path`
- `url`
- `symbol` - Erlang/Elixir atoms, Ruby symbols, Clojure keywords
Copy link
Member Author

Choose a reason for hiding this comment

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

this is already in use by the ruby highlights for ruby symbols so I've just added documentation for it here

for everything else I was able to find suitable scopes that were already documented

@the-mikedavis
Copy link
Member Author

the-mikedavis commented Oct 15, 2021

I think this is pretty close to being ready. There are some additional highlights I'd like to add w.r.t. function captures and maybe typespecs.

There's some odd behavior I haven't seen in helix though that may be caused by something in the new grammar

helix

(sry for the low quality gif)

The scopes seem to get into a bad state with an incremental change. I suspect some kind of precedence rule is at play here, I'll try to dig deeper

(edit: I can reproduce the behavior in the tree-sitter playground, so must be a bug in tree-sitter)

@the-mikedavis the-mikedavis force-pushed the official-elixir-tree-sitter branch from 69bd5d8 to 4d8eb09 Compare October 17, 2021 15:50
@the-mikedavis the-mikedavis marked this pull request as ready for review October 17, 2021 15:53
Comment on lines +1 to +16
; The following code originates mostly from
; https://github.com/elixir-lang/tree-sitter-elixir, with minor edits to
; align the captures with helix. The following should be considered
; Copyright 2021 The Elixir Team
;
; Licensed under the Apache License, Version 2.0 (the "License");
; you may not use this file except in compliance with the License.
; You may obtain a copy of the License at
;
; https://www.apache.org/licenses/LICENSE-2.0
;
; Unless required by applicable law or agreed to in writing, software
; distributed under the License is distributed on an "AS IS" BASIS,
; WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
; See the License for the specific language governing permissions and
; limitations under the License.
Copy link
Member Author

Choose a reason for hiding this comment

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

Is this ok to add? I don't really know what to do here since I'm effectively copying https://github.com/elixir-lang/tree-sitter-elixir/blob/c3b82ff9160d4780afbba5249b9de884e379c8dd/queries/highlights.scm and making minor edits to align the scopes.

I think it's relatively common practice to do the copying part, especially for queries from the repositories in the @tree-sitter organization, but the attribution part is not very common (e.g. https://github.com/nvim-treesitter/nvim-treesitter/blob/58dd95f4a4db38a011c8f28564786c9d98b010c8/queries/c_sharp/highlights.scm).

@jonatanklosko do you have thoughts on this?

Choose a reason for hiding this comment

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

This looks good to me! Generally I don't mind if it's just a link for the reference, but from formal point of view that's probably the proper way :)

cc @josevalim

@the-mikedavis
Copy link
Member Author

Ok I think this is good and ready for review: I've been using it for a bit and it seems pretty stable. The only outstanding problem on my mind is that incremental change bug, but I have an issue up in tree-sitter for that. It's a pretty rare behavior and it's possible to un-break the tree by making some minor edits to the effective code (or by closing and re-opening the editor), so I'm ok with it.

Copy link
Member

@archseer archseer left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉

@archseer archseer merged commit a03b125 into helix-editor:master Oct 18, 2021
@the-mikedavis the-mikedavis deleted the official-elixir-tree-sitter branch October 18, 2021 12:25
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