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 command wrapping ocamlformat #10929

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

Conversation

gridbugs
Copy link
Collaborator

@gridbugs gridbugs commented Sep 17, 2024

Adds a command dune tools exec ocamlformat which downloads and builds ocamlformat as a dev tool before running it, passing all positional arguments to the ocamlformat executable. This is intended to be run by text editors.

@gridbugs
Copy link
Collaborator Author

I found an issue with this where if opam is installed but no switch is set then dune tools exec ocamlformat won't fall back to a globally-installed instance of ocamlformat. Will fix.

Adds a command `dune tools exec ocamlformat` which downloads and
builds ocamlformat as a dev tool before running it, passing all
positional arguments to the ocamlformat executable. This is intended
to be run by text editors.

Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
@gridbugs
Copy link
Collaborator Author

I found an issue with this where if opam is installed but no switch is set then dune tools exec ocamlformat won't fall back to a globally-installed instance of ocamlformat. Will fix.

This is now fixed.

@gridbugs
Copy link
Collaborator Author

I don't understand why the opam switch create test is failing. I can reproduce it on the main branch locally but error doesn't make sense to me based on the vendored copy of pp. In any case it's not related to this change.

@rgrinberg
Copy link
Member

I think we just need to update our vendored copy of pp

failing that from the PATH. This is necessary so that editors
configured to run ocamlformat via dune can still be used to format
ocaml code outside of dune projects. *)
let run t args env =
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that we need this fallback. Or perhaps put differently, I'm not sure dune should be responsible for it. I could imagine that editors might want to perhaps suggest to the user if they would like to switch to dune's package management to make this command work, or perhaps tell them there's an opam fallback available. Or perhaps the user should have a way to prefer to use opam and dune shouldn't even try package management at all.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess the question is how much sophistication we expect from editor plugins. Today in emacs and neovim there's a way to set a command to run to invoke ocamlformat (overriding the default command which is just ocamlformat). My goal adding fallback behaviour to dune was to avoid needing to change editor plugins to become explicitly aware of dune dev tools. I claim that centralizing fallback behaviour within dune rather than implementing it in each ocamlformat editor plugin will be less work and easier to maintain. Centralizing this behaviour in dune also means that users can try out dune-managed ocamlformat (and soon ocamllsp too) today by just updating their editor config and don't need to wait for support to land in their editor plugins.

Copy link
Member

Choose a reason for hiding this comment

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

It would be easier to maintain if there was one fallback behavior which was correct in all cases. I don't think that's the case though. In the vscode plugin for example, a sandbox is selected explicitly, and once the user selects the sandbox all fallback options don't really make sense.

With that being said, you can keep the current behavior, but it's very likely that it will be subject to change once we evolve this feature.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah I did not realize that vscode's ocaml plugin doesn't allow the user to customize the commands used to run ocamlformat or ocamllsp and has a built-in understanding of opam switches. From playing around with it a bit it seems like the plugin will need to be updated to allow dune to manage these tools no matter what the ui looks like on the dune side.

For emacs and vim the plugins for lsp and ocamlformat are lower level and let you customize the command that runs the tool. Generally I have found it necessary to customize the command to be opam exec <tool> -- and I'm trying to make it so all users need to do to adopt dune dev tools is change the command to dune tools exec <tool> -- without needing to add sophistication to plugins.

Copy link
Member

Choose a reason for hiding this comment

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

Right but I think that communicating the editor a "dev environment" is better than having to teach each tool about dune tools and its fallback behavior. Configuring every single command separately inside the editor rather than just telling it "here's a dev environment, fetch what you need" seems a bit simpler. For example, ocamllsp needs to invoke ocamlformat. Do you expect it to be modified to run dune tools

This is what I implemented in vscode from my experience learning from esy at least.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that makes sense for vscode but I'm not so sure about other editors. For example in neovim there is no need for an ocaml-specific lsp plugin. The general lsp client plugin just requires a command to start the lsp server for each language, so for ocaml you can just tell it to run opam exec ocamllsp --. There's also no plugin necessary for ocamlformat. I use a more general formatting plugin called "neoformat" and configure it with a command to run to format files with an ocaml extension.

I agree that the fact that ocamllsp invokes ocamlformat does complicate the editor integration story a bit but I'd be interested to find a way to resolve this without needing to add sophistication to editor plugins. One option would be to add an argument to ocamllsp for overriding the command to run ocamlformat.

Copy link
Member

Choose a reason for hiding this comment

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

Btw how does your proposal work with other binaries in ocamlformat? For example, ocamllsp also uses ocamlformat-rpc to format tooltips.

One option would be to add an argument to ocamllsp for overriding the command to run ocamlformat.

I think that's exactly why going down this route is not what we'd like to do. It is advantageous for dev tools to be able to use each other to provide additional functionality. We shouldn't be introducing barriers for this to work. Adding new dev tools that leverage existing ones should be something to facilitate instead.

Here's an alternative proposal, we create a "dev" environment that sets the PATH to include all dev tools. Then, we can run any dev tool as dune tools exec -- <tool name>.

Copy link
Member

Choose a reason for hiding this comment

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

Created a new ticket to track this: #10964

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here's an alternative proposal, we create a "dev" environment that sets the PATH to include all dev tools. Then, we can run any dev tool as dune tools exec -- .

I like this idea. Rather than needing to wrap every tool with an explicit command, we just make dune tools exec -- <command> execute <command> in an environment with all the dev tools in PATH. It would also need to make sure that if the command is running a dev tool that isn't currently installed, then it gets locked and installed.

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.

2 participants