Auto format `dune-project` files with `dune fmt` #10716

maxRN · 2024-07-10T14:37:40Z

Hi!

This PR enables automatic formatting of dune-project files when running dune fmt. This should close #3223. Note that there were some concerns raised on that issue regarding the formatting, but those should have been addressed by #3928, see also #2291.

Note: I'm new to OCaml, especially working on such a mature codebase so there will probably a lot of mistakes in my submission. I hope it can still be of use and I'm more than happy to learn and address any problems with this PR.

Signed-off-by: Max Große <[email protected]>

maxRN · 2024-07-10T14:38:35Z

src/dune_rules/format_rules.ml

+    | true ->
+      Dune_project.file project
+      |> Memo.Option.iter ~f:(fun path ->
+        if String.equal "_build/default" (Path.Build.to_string dir)


I'm not happy with this if check for the hard-coded build directory, but I didn't find a way to check if we are in the "_build/default" directory other than this.

How about checking it for equality against Context.build_dir?

maxRN · 2024-07-10T15:31:00Z

EDIT: No longer relevant.
~~I'm not sure why the Format CI step fails. It runs nix develop .#fmt -c make fmt right? When I run this on my machine I don't get any errors so I'm not sure why it's failing in CI.~~

emillon · 2024-07-11T12:10:53Z

Thanks.
I can't give a full review at the moment but here are a few remarks in the meantime:

In terms of design, I don't think that this should be tied to the formatting of dune files. Maybe it's useful to be able to disable the formatting of just dune-project files, for example.
This will need to be versioned: if (lang dune) is < (3, 17), do not format these files, otherwise do format them by default.
I'm not sure the formatting has been fixed enough to work well with dune-project files, but that's something we can try and improve later.

maxRN · 2024-07-11T19:05:15Z

Thanks. I can't give a full review at the moment but here are a few remarks in the meantime:

In terms of design, I don't think that this should be tied to the formatting of dune files. Maybe it's useful to be able to disable the formatting of just dune-project files, for example.

This will need to be versioned: if (lang dune) is < (3, 17), do not format these files, otherwise do format them by default.

I'm not sure the formatting has been fixed enough to work well with dune-project files, but that's something we can try and improve later.

Thanks for the feedback! I'll take another look

Signed-off-by: Max Große <[email protected]>

maxRN · 2024-07-14T11:40:42Z

test/blackbox-tests/test-cases/formatting/feature.t/run.t

@@ -190,7 +190,8 @@ Sometimes, the suggestion is to just remove the configuration.
  5 | (using fmt 1.2)
      ^^^^^^^^^^^^^^^
  Error: Starting with (lang dune 2.0), formatting is enabled by default.
-  To port it to the new syntax, you can delete this part.
+  To port it to the new syntax, you can replace this part by:
+  (formatting (enabled_for dune ocaml reason))


Unsure whether we should extend the help message by mentioning the differences between > 3.17 dune and 3.17+
i.e. that you can remove the stanza, but that it will also format dune-project files.

maxRN · 2024-07-14T11:52:27Z

@emillon I've added a version check so it will only automatically format from 3.17 (?) onwards. There's no explicit way to disable just formatting for dune-project files, but just like before users can just specify the file types that they want to be formatted automatically and not include dune-project.
One more thing I thought about while implementing this: I think it would make sense to also include dune-workspace files in this PR. I don't know what the state on their formatting is, but it would be a lot easier to add support for both file types in one change then having to go back later and add support.

rgrinberg · 2024-07-14T14:20:31Z

src/dune_lang/format_config.ml

@@ -14,27 +14,35 @@ module Language = struct
    type t =
      | Dialect of string
      | Dune
+      | DuneProject


Dune_project is the convention

rgrinberg · 2024-07-14T14:21:18Z

Can you enable this feature for dune itself? IIRC, there were some problems with preserving multi line quoted strings in the dune language.

maxRN · 2024-07-14T14:31:01Z

Can you enable this feature for dune itself? IIRC, there were some problems with preserving multi line quoted strings in the dune language.

Sorry, I don't think I understand the question. What feature exactly are you referring to?

rgrinberg · 2024-07-14T14:33:01Z

Dune itself has a dune-project file. It would be good to see how this PR works to auto-format it.

Signed-off-by: Max Große <[email protected]>

maxRN · 2024-07-14T14:43:58Z

Ahh, I see what you mean. I added the formatted dune-project file for reference. NOT intended to be included in the final PR. You are right, there are some problems with multiline strings - the newlines are output verbatim instead of being actual newline characters.
My gut feeling is that this should be an easy fix, do you want me to tackle this in this PR as well or create a new one so that we can discuss this separately?

rgrinberg · 2024-07-14T15:38:37Z

If you could fix it in a separate PR that would be great. It's really an issue for all dune files, but project files include large description strings fairly often.

maxRN · 2024-07-17T17:43:15Z

Alright, I'm investigating some possible solutions and will try to create a separate draft PR in the coming days.

maxRN added 2 commits July 10, 2024 16:00

doc: fix typo

087ea7e

Signed-off-by: Max Große <[email protected]>

feat(fmt): auto format dune-project files

015b715

Signed-off-by: Max Große <[email protected]>

maxRN commented Jul 10, 2024

View reviewed changes

maxRN added 2 commits July 13, 2024 12:06

feat: enable dune-project formatting for dune <= 3.17

d060db8

Signed-off-by: Max Große <[email protected]>

fix: typo in example

c47472e

Signed-off-by: Max Große <[email protected]>

maxRN force-pushed the dune-fmt-dune-project-files branch from 416edcc to c47472e Compare July 13, 2024 10:06

maxRN added 5 commits July 13, 2024 12:09

fix: don't hardcode build dir

8215838

Signed-off-by: Max Große <[email protected]>

doc: update formatting docs

bc18985

Signed-off-by: Max Große <[email protected]>

test: test disabled dune-project formatting

d968084

Signed-off-by: Max Große <[email protected]>

fix: handle empty field case pre-3.17

f3d2de2

Signed-off-by: Max Große <[email protected]>

fix: add new dune 2.0 variant

a8f0ede

Signed-off-by: Max Große <[email protected]>

maxRN commented Jul 14, 2024

View reviewed changes

rgrinberg reviewed Jul 14, 2024

View reviewed changes

maxRN added 2 commits July 14, 2024 16:33

rename: DuneProject -> Dune_project

e773978

Signed-off-by: Max Große <[email protected]>

test!: format dune-project with format-dune-file

befce10

Signed-off-by: Max Große <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auto format `dune-project` files with `dune fmt` #10716

Auto format `dune-project` files with `dune fmt` #10716

maxRN commented Jul 10, 2024 •

edited

Loading

maxRN Jul 10, 2024

rgrinberg Jul 11, 2024

maxRN commented Jul 10, 2024 •

edited

Loading

emillon commented Jul 11, 2024

maxRN commented Jul 11, 2024

maxRN Jul 14, 2024

maxRN commented Jul 14, 2024

rgrinberg Jul 14, 2024

rgrinberg commented Jul 14, 2024

maxRN commented Jul 14, 2024

rgrinberg commented Jul 14, 2024

maxRN commented Jul 14, 2024

rgrinberg commented Jul 14, 2024

maxRN commented Jul 17, 2024

Auto format dune-project files with dune fmt #10716

Are you sure you want to change the base?

Auto format dune-project files with dune fmt #10716

Conversation

maxRN commented Jul 10, 2024 • edited Loading

maxRN Jul 10, 2024

Choose a reason for hiding this comment

rgrinberg Jul 11, 2024

Choose a reason for hiding this comment

maxRN commented Jul 10, 2024 • edited Loading

emillon commented Jul 11, 2024

maxRN commented Jul 11, 2024

maxRN Jul 14, 2024

Choose a reason for hiding this comment

maxRN commented Jul 14, 2024

rgrinberg Jul 14, 2024

Choose a reason for hiding this comment

rgrinberg commented Jul 14, 2024

maxRN commented Jul 14, 2024

rgrinberg commented Jul 14, 2024

maxRN commented Jul 14, 2024

rgrinberg commented Jul 14, 2024

maxRN commented Jul 17, 2024

Auto format `dune-project` files with `dune fmt` #10716

Auto format `dune-project` files with `dune fmt` #10716

maxRN commented Jul 10, 2024 •

edited

Loading

maxRN commented Jul 10, 2024 •

edited

Loading