Skip to content

Conversation

davesnx
Copy link

@davesnx davesnx commented May 7, 2025

Small PR to format mlx files with ocamlformat-mlx.

In mlx we encourage dune fmt via dune dialects, but the lsp runs those formatters outside of dune.

(dialect
 (name mlx)
 (implementation
  (extension mlx)
  (merlin_reader mlx)
  (format
   (run ocamlformat-mlx %{input-file}))
  (preprocess
   (run mlx-pp %{input-file}))))

@davesnx davesnx changed the title Add ocamlformat mlx Add ocamlformat-mlx for .mlx files May 7, 2025
@davesnx davesnx marked this pull request as ready for review May 15, 2025 11:40
(* TODO: Unsure about this, keeping it empty for now *)
| Mlx ->
(match kind with
| Intf -> [ re; ml; mly; mll ]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
| Intf -> [ re; ml; mly; mll ]
| Intf -> [ re; ml; mly; mll; mlx ]

as we want to be able to switch to .mlx from .mli if it is present

Copy link
Collaborator

Choose a reason for hiding this comment

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

But here we are matching on an "Mlx Intf", which is not an mli file, right ? I am not sure that case happens at all. But nothing really bad could happen from having too many of such rules...

Copy link
Member

Choose a reason for hiding this comment

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

looking at the surrounding code, I thought the kind belongs to the origin, so we are on interface file and want to switch to the implementation, which can be .re, .ml, .mly, mll, and now .mlx

in
match Syntax.of_fname fname with
| Dune | Cram -> []
(* TODO: Unsure about this, keeping it empty for now *)
Copy link
Member

Choose a reason for hiding this comment

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

not clear what's the TODO about

@voodoos
Copy link
Collaborator

voodoos commented May 28, 2025

@davesnx I am not familiar with ocamlformat-mlx, but overall this looks correct.

However the CI is failing with a non-exhaustive pattern matching error, could you fix that ?

@davesnx
Copy link
Author

davesnx commented May 29, 2025

Embarrassing, I would push this forward. Thanks for the review @voodoos.

I wanted to test this PR manually before merging as well, is it as easy as pinning to ensure everything works or do I need something else?

@voodoos
Copy link
Collaborator

voodoos commented May 29, 2025

I wanted to test this PR manually before merging as well, is it as easy as pinning to ensure everything works or do I need something else?

I would expect so, please report if it doesn't :-)

…rmat-mlx

* 'master' of github.com:/ocaml/ocaml-lsp:
  nix: updates (ocaml#1550)
  refactor: get rid of a bunch of [Stdune.String] uses (ocaml#1551)
  Deleted a dead link (ocaml#1549)
  Yojson 3 compat (ocaml#1534)
  Prepare release 1.23.0 (ocaml#1539)
  Delegate outline generation to Merlin (ocaml#1529)
  Fix yojson constraint (ocaml#1538)
  chore: remove all the stdune [O] references (ocaml#1483)
  Handle new kind of outline symbol (ocaml#1527)
@davesnx davesnx force-pushed the Add-ocamlformat-mlx branch from ad4d4a8 to 0988f2b Compare September 16, 2025 08:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants