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

Add reason language to ast_editor #1685

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pedrobslisboa
Copy link

@pedrobslisboa pedrobslisboa commented Dec 11, 2024

Description

This PR adds reasonml to ast_editor to be able to see AST and Pp

Screenshots

Working with both ocaml and reason

Captura de Tela 2024-12-11 às 17 45 26
Captura de Tela 2024-12-11 às 18 24 14

How to test

@rgrinberg
Copy link
Contributor

Can we think of a way of having this feature without making the plugin on reason? In the past, I've found the the dependency on reason to be rather disruptive to development. I would much prefer if we had a way to extend this plugin so that you could reason support externally.

@davesnx
Copy link

davesnx commented Dec 12, 2024

Do you have any idea of how "extending the plugin" would work in terms of VSCode packaging, APIs, etc.? Or are you talking about like an ocamlmerlin-reason integration, or to have "reason" installed in the host?

The OCaml platform already supports syntax highlight for Reason files, and I personally find it useful to have a better integration with Reason, such as converting between syntaxes built-in visiting the AST or ppx output.

What are the problems of reason as a dependency on development? Maybe we can fix those?

@rgrinberg
Copy link
Contributor

The problem is that the reason package is not actively maintained, and the version in opam has historically lagged behind. This is particularly painful for a package that depends on the internals of the compiler.

I don't really have a concrete idea on how to work around this. I was hoping that you would think of something. One simple approach is to call out to a reason binary to dump the parsetree

@davesnx
Copy link

davesnx commented Dec 12, 2024

Oh, reason has been very maintained this past years (since 2022), and we plan to keep it that way: https://github.com/reasonml/reason/releases releases are pushed to opam only.

Sure, there might be other options, but it faces problems with compatibility (that a package manager like opam fixes for us) and other issues.

Happy to commit to keep ocaml-platform up-to-date as dependency of Reason.

@rgrinberg
Copy link
Contributor

Happy to hear reason is being maintained again. While that certainly makes this more risky, the fact that reason is barely used, makes the reward for taking such a risk less enticing.

Merlin and lsp both support reason without a hard dependency. I'd hate to improvish the vscode extension with such a dependency.

@pedrobslisboa pedrobslisboa force-pushed the feat/add-reason-to-ast-n-pp branch 3 times, most recently from a9db2e6 to 12e48b6 Compare December 12, 2024 15:42
@pedrobslisboa
Copy link
Author

pedrobslisboa commented Dec 12, 2024

@rgrinberg, it turned out that I was overcomplicating it; Ast can handle .re
Just changed it.

src/ast_editor.ml Outdated Show resolved Hide resolved
src/ast_editor.ml Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@smorimoto
Copy link
Collaborator

@pedrobslisboa You have to add a change log entry as well.

@pedrobslisboa
Copy link
Author

@pedrobslisboa You have to add a change log entry as well.

True, thank you. I'm going to fix it as well

@pedrobslisboa pedrobslisboa force-pushed the feat/add-reason-to-ast-n-pp branch from 8d013db to c43108a Compare December 14, 2024 20:32
@pedrobslisboa
Copy link
Author

@smorimoto Done, ready for review

@pedrobslisboa pedrobslisboa force-pushed the feat/add-reason-to-ast-n-pp branch 3 times, most recently from 0f60ca2 to 55e2a05 Compare December 15, 2024 14:12
@smorimoto smorimoto requested a review from Copilot December 28, 2024 17:50

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 2 changed files in this pull request and generated no comments.

Files not reviewed (1)
  • src/ast_editor.ml: Language not supported
@pedrobslisboa
Copy link
Author

@rgrinberg @smorimoto 🏓

@smorimoto smorimoto force-pushed the feat/add-reason-to-ast-n-pp branch from a018b3a to a8ab852 Compare January 14, 2025 16:42
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.

4 participants