-
Notifications
You must be signed in to change notification settings - Fork 39
Add Odin support #188
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
Add Odin support #188
Conversation
|
This looks great so far, but oof, I have no idea how to read Odin. There's also no Odin support in my tree-sitter visualizer, so debugging this will take some time. I'd be happy to merge this as is though if you want it in now? |
|
Hey, I'd like to spend more time on it, just got distracted with the holidays (Happy New Year btw 🎉). I'm reading up on Tree-sitter, I wanted to learn how to make a parser anyway, then I'll know for sure if it's limited by the grammar or just needs a better query. |
|
Happy new year! Sounds good. I find that adding tests helps with the iteration speed a lot. Happy to answer any questions, but I have no experience with Odin specifically 🙂 |
|
Oh, I had some tests already, just didn't include them in the PR. They immediately flagged the duplication and I figured better do a proper fix instead of figuring how to suppress those and submitting a partial solution. Odin is quite simple, no-nonsense, boring in a good way and the community is awesome! If you find a spare hour, I would encourage you to look up some interviews with the creator of Odin @gingerBill and give the language a try. I found that it simplicity brought back joy to my programming life, serving as a nice break from the Web and Rust stuff. |
|
Didn't mean to do that. Sorry |
|
I wanted to tidy things up and force-pushed your current From what I was able to gather, grammar is indeed needed to be changed, I ended up forking it and publishing it under a different crate since the original doesn't seem to be active. I also merged changes from one of the more active forks, not sure that it was necessary for Codebook specifically, and in hindsight it wasn't, but I think it shouldn't hurt. If you want to verify that I didn't do anything funny, you can: git clone [email protected]:Igonato/tree-sitter-odin-codebook.git
cd tree-sitter-odin-codebook
tree-sitter generateAnd check the diff. My I have some thoughts how we can better handle grammars in this project, I'll write that a new issue since it's unrelated to the PR. Cheers! |
|
Rebased on the latest main and ran |
|
Hey! Nice, I've had to fork a few grammars myself, which is kind of a pain. I saw your proposal issue. I need to investigate a bit, but I'll reply to that issue. Anyway, this looks great! Thank you. |
Adding Odin support (#182). Currently there are false positives with CONSTANTS in enum variant values and bit_fields size:
Can use another pair of eyes, I might be missing a way to make a query that can handle this case. It is also possible that's the grammar needs changing. Grammar for reference: https://github.com/tree-sitter-grammars/tree-sitter-odin/blob/master/grammar.js
What do you think?