Skip to content

dune format-dune-file: use Dune lang version of current Dune project #11865

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

nojb
Copy link
Collaborator

@nojb nojb commented May 26, 2025

This PR extends dune format-dune-file so that the Dune version of the current Dune project is used when formatting. If a file is passed as argument, then it is the Dune project that owns the file that is used. If formatting standard input, it is the Dune project containing the current directory that is used.

This fixes the isssue discussed in the thread starting at #10892 (comment).

This also opens the door to honouring the (formatting) settings of the current Dune project (cc @Khady, cf #3516).


val create_exn : default_is_cwd:bool -> specified_by_user:string option -> t
val create_exn
: ?from:string
Copy link
Member

Choose a reason for hiding this comment

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

How about just always passing this from argument? I don't think there's many callers and it would help us be assured that you didn't forget to set this somewhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea. Fixed, thanks.

Signed-off-by: Nicolás Ojeda Bär <[email protected]>
bin/common.mli Outdated
@@ -46,7 +46,7 @@ end

Return the [Common.t] and the final configuration, which is the same as the one
returned in the [config] field of [Dune_rules.Workspace.workspace ()]) *)
val init : Builder.t -> t * Dune_config_file.Dune_config.t
val init : ?root:Workspace_root.t -> Builder.t -> t * Dune_config_file.Dune_config.t
Copy link
Member

Choose a reason for hiding this comment

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

Same for ~root here. Let's just pass it always

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 4d7a096. I ended up using a separate function instead of making the argument mandatory, so as to avoid too much code duplication.

@nojb
Copy link
Collaborator Author

nojb commented May 29, 2025

I added a warning when using dune format-dune-file inside Dune, as this may not give the expected results (root detection behaves differently in that case) and the (format-dune-file) action seems a better option in that case. dfc6027

Signed-off-by: Nicolás Ojeda Bär <[email protected]>
@nojb nojb force-pushed the format-dune-file-with-project-version branch from 71304a1 to 209091f Compare May 29, 2025 19:21
let cwd = Sys.getcwd () in
let find from =
let cwd =
if Filename.is_relative from then Filename.concat (Sys.getcwd ()) from else from
Copy link
Member

Choose a reason for hiding this comment

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

How about Fpath.initial_cwd? The old code didn't use it before, but at least it only called getcwd once.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good idea, done.

Signed-off-by: Nicolás Ojeda Bär <[email protected]>
@nojb nojb force-pushed the format-dune-file-with-project-version branch from ceb3d10 to cc5a628 Compare May 30, 2025 06:39
Signed-off-by: Nicolás Ojeda Bär <[email protected]>
@rgrinberg
Copy link
Member

Tests are failing in this one

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.

2 participants