-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
languages: add koka #8727
languages: add koka #8727
Conversation
Oh, one other thing I should probably explain: the |
This looks great! I've looked through some example koka code, and noticed the following things, probably something for future improvements. Is the "scoped" keyword missing? No "local variable" support yet (https://tree-sitter.github.io/tree-sitter/syntax-highlighting#local-variables) |
Yes, thanks for catching that @chtenb! I have a fix on this branch here: https://github.com/mtoohey31/tree-sitter-koka/tree/fix/scoped, would you mind taking a look? The places where
I've updated the locals queries to handle the example above in d17962b. Let me know if you spot anything else that's being missed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The places where scoped can be used aren't described in the grammar spec, but it's clearly supported by the compiler.
Yeah, I see. It looks like the language is in flux and the spec isn't fully keeping up.
Are linear
effects already in the language? If so, that keyword can also be added https://koka-lang.github.io/koka/doc/book.html#sec-linear
I think I looked through all language samples, and there was nothing else that stood out to me.
EDIT:
It would be nice to have var
declarations stand out from other (immutable) identifers such as val
s and function parameters. Perhaps we can find a suitable scope for these? The list of scopes at our disposal is not particularly accomodating unfortunately: https://docs.helix-editor.com/themes.html. What about variable.other
? Or special
?
I do have
Yeah it looks like we don't have much selection... |
I guess we should leave it for now then
The reason I thought of this is because this is how F# highlighting works in Visual Studio. Similar to Koka and many functional languages, immutability is the default. Mutable variables are highlighted by a distinctive color, which makes it easier to grasp the code flow when reading source code. I will create a separate issue for this. |
Turns out there is an open issue already #6659 Moreover, the rust highlights have an open TODO for this feature: helix/runtime/queries/rust/highlights.scm Line 183 in bff7fc8
The suggested name is |
887df4a
to
31c638d
Compare
Alright, I've pushed a small fix for ntlappexpr highlighting, and I've added a TODO comment for adding the mutability highlight. I agree with your suggested solution @chtenb, but I think it should probably be a separate PR cause it'll involve a lot of theme updates, as well as updates to the Rust and Kotlin highlight queries at the very least. Does that sound fair? If so, I think this PR is ready. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. LGTM
I've noticed a minor issue in the indent queries. When entering a new line after something like below, the cursor starts indented.
|
674c252
to
a7d6d8e
Compare
Thanks, I've fixed the function highlighting and resolved the merge conflict. I'm working on the indent query. I can reproduce it but haven't figured out how to fix it yet. |
I noticed another undocumented keyword missing:
|
Here is a somewhat similar situation
I think it would make sense to have two or at least one level of indentation here. |
I've added a couple missing keywords in 9c2af05. You may want to take a look at mtoohey31/tree-sitter-koka@4eef46e to see if that's all covered by koka-lang/koka#364. I'll fix the merge conflicts again in a minute. Still working on the indentation. |
bd6e42e
to
88b15d3
Compare
Based on how this is handled in other languages, I've chosen to fix this by highlighting it as a |
I haven't seen this used, no. I just noticed that the compiler accepts it, but adding the braces there makes sense for readability purposes. If the grammar spec doesn't allow it either, putting this in the "won't fix" bin is fine I think. |
Sounds good! Sorry about all the 👍 reactions lol, I've been using them to mark off what I've addressed. I believe I've fixed everything as of a26cb86. |
Cool! No worries, I don't get notifications about reactions or anything |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a minor comment about a field that can be removed, otherwise this looks good to me
It could use a master-merge or rebase though to resolve the conflicts
languages.toml
Outdated
scope = "source.koka" | ||
injection-regex = "koka" | ||
file-types = ["kk"] | ||
roots = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This key can be dropped since this is the default value now (#8803)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, should be fixed as of 62bda67.
Co-authored-by: Pascal Kuthe <[email protected]>
Co-authored-by: Pascal Kuthe <[email protected]>
Co-authored-by: Pascal Kuthe <[email protected]>
Co-authored-by: Pascal Kuthe <[email protected]>
Co-authored-by: Pascal Kuthe <[email protected]>
Co-authored-by: Pascal Kuthe <[email protected]>
Co-authored-by: Pascal Kuthe <[email protected]>
Co-authored-by: Pascal Kuthe <[email protected]>
Co-authored-by: Pascal Kuthe <[email protected]>
This PR adds support for Koka. It includes a grammar, highlights, and indents queries.
There isn't currently a complete language server, so I haven't included one (though there is an unmerged PR which implements one, so if that gets merged I'll create a follow-up PR).
The grammar isn't completely up-to-date with the latest version of the language: it doesn't include the newfip
/fbip
keywords added in 2.4.2 because the official grammar specification hasn't been updated to include them yet either. I intend to ask about this upstream; depending on when the official grammar spec is updated I may update the tree-sitter grammar's version in this PR before it's merged, or I might do that in a follow-up PR, if that's ok.Edit: nevermind, I found more details on another upstream branch, so this should be fixed now.
Here's a screenshot with the gruvbox theme: