Add support for the OCaml language#157
Conversation
Co-authored-by: Tim McGilchrist <timmcgil@gmail.com> Co-authored-by: PizieDust <playersrebirth@gmail.com>
Co-authored-by: Tim McGilchrist <timmcgil@gmail.com> Co-authored-by: PizieDust <playersrebirth@gmail.com>
Co-authored-by: Tim McGilchrist <timmcgil@gmail.com> Co-authored-by: PizieDust <playersrebirth@gmail.com>
- Fix cl-assert it is checking for a symbol not a list. - This converts Combobulate names (with hyphens) to tree-sitter language names (with underscores). This matches the grammar naming for ocaml_interface and ocaml_type, which matches the .so/dylib library names that tree-sitter creates. Co-authored-by: Tim McGilchrist <timmcgil@gmail.com> Co-authored-by: PizieDust <playersrebirth@gmail.com>
Co-authored-by: Tim McGilchrist <timmcgil@gmail.com> Co-authored-by: PizieDust <playersrebirth@gmail.com>
Co-authored-by: Tim McGilchrist <timmcgil@gmail.com> Co-authored-by: PizieDust <playersrebirth@gmail.com>
Co-authored-by: Tim McGilchrist <timmcgil@gmail.com> Co-authored-by: PizieDust <playersrebirth@gmail.com>
Co-authored-by: Tim McGilchrist <timmcgil@gmail.com> Co-authored-by: PizieDust <playersrebirth@gmail.com>
Co-authored-by: Tim McGilchrist <timmcgil@gmail.com> Co-authored-by: PizieDust <playersrebirth@gmail.com>
Co-authored-by: Tim McGilchrist <timmcgil@gmail.com> Co-authored-by: PizieDust <playersrebirth@gmail.com>
Co-authored-by: Tim McGilchrist <timmcgil@gmail.com> Co-authored-by: PizieDust <playersrebirth@gmail.com>
Co-authored-by: Tim McGilchrist <timmcgil@gmail.com> Co-authored-by: PizieDust <playersrebirth@gmail.com>
Co-authored-by: Tim McGilchrist <timmcgil@gmail.com> Co-authored-by: PizieDust <playersrebirth@gmail.com>
Co-authored-by: Tim McGilchrist <timmcgil@gmail.com> Co-authored-by: PizieDust <playersrebirth@gmail.com>
|
Looks great! Good effort. Any challenges / general feedback on the process of writing this? I know O'Caml's a very complex language.
Sorry that explains the navigate into lists thing. You'll have to hoist that into the "declare" machinery and make it a formal toggle there then instead so it uses
By all means add your names next to the language in the README! I do not see any edit tests for things like cloning. They are important because they use the movement machinery to select and edit. The snag here is that dumb things like whitespace and newlines are properly preserved; this is notoriously hard to get right without tests to ensure you do not break something in, you know, YAML and Go or something. Minor thing. Should be quick to generate by copy-pastaing how the existing tests do it. |
Thank you. Just updated it to have a test for cloning. |
Please can you give more clarification about what you mean by hoisting it into the declare machinery and adding the toggle feature? |
|
Sure. The You can probably do this also with the tree-sitter naming thing. I don't like the idea of having to wrap everything in complex name checking code everywhere - I feel this can be done centrally, in one place, without polluting random bits of code. But I haven't actually checked every code path. |
|
Did another sweep. Aside from the minor issues mentioned -- and assuming all the tests pass, I am very sorry I have not checked -- we can get this merged once they're done. Let me know if you need advice with how to do this. |
|
Thanks @mickeynp , So all the tests currently pass excellently: I'll be updating this PR soon with the changes according to your remarks |
Some changes are proposed in #158 |
|
Thanks. Once the imenu thing is moved out we can get it all merged in. Don't forget to update the README with your names if you feel like it. |
|
To be clear: the imenu treesit.el customisation is not something I want in Combobulate. it's major mode specific code and it belongs in whatever package uses it. The other problem is the naming scheme of the tree-sitter symbols. You made a bunch of changes to combobulate to handle that. I do not like this approach. There should be one singular lookup in Combobulate for tree-sitter's language name if you do not wish to use the canonical one. (It will also need one or two tests to ensure it works.) |
We have updated this PR removing imenu support and also updated #158 to only focus on |
d40f077 to
73e3796
Compare
|
Great. Thanks. I've merged it! |
Hi @mickeynp, could you please explain this a bit more? My understanding is that you want us to revert the modifications we made in other to support ocaml and ocaml_interface grammars? |
|
Hi. I added a new comment just now. |
I can't find the comment you added, could you link me to it |
|
Oh that's weird. https://github.com/mickeynp/combobulate/pull/157/changes#r3031991590 here you go |
I think GitHub may be broken. output.mp4 |
|
Anyway. your changes in interface.el and elsewhere: any and all code that has been added that does additional tree-sitter language/name checking must be removed and put into one centralised function that is itself called ideally just one in the core. |
This PR adds support for OCaml
As is often the case when working with OCaml tools, a language with more structural elements than typical languages and capable of potentially _infinite levels of recursion within each items, the implementation is somewhat more complex than for less flexible languages, hence the various shortcuts in the definition of hierarchical and sibling procedures.
Support is added for:
.ml) & interface files (.mli)Navigation supported includes:
C-M-h)M-a/M-e)C-M-n/C-M-p)C-M-u/C-M-d)Tests files are written in:
Notes:
We added a hook to disable the hierarchy navigation fallback of jumping to the next delimiter, as this gives a poor navigation experience for OCaml code.
In the source tree, you'll find an example of a bridge (
tuareg-treesit) that demonstrates how to use combobulate with a mode that isn't based on Treesitter. This is important for the OCaml community because many people have continued to use Tuareg (which isn't Treesitter-based).Writen mostly by Tim McGilchrist and PizieDust (with some small cosmetic helps from Xavier Van de Woestyne)