Tweak dune file formatting: avoid pure vertical layout when formatting wrapped lists #10892

nojb · 2024-09-06T16:55:41Z

The effect of the change is illustrated in the tests.

rgrinberg

LGTM. Changes entry please

Khady · 2024-09-07T11:36:38Z

Thanks! It would definitely help with run. I don't have a real opinion about the impact on other wrapped lists. From the diff it generally looks good. Two points where it could look worse are modules and libraries where it can be useful to have things kept in a alphabetical order to ease reading. I tend to think that the horizontal layout would be fine anyway.

nojb · 2024-09-07T12:02:25Z

I don't have a real opinion about the impact on other wrapped lists.

Unfortunately, the formatter works at the level of s-expressions; so we don't really know the difference between "run" and other lists, so using different formatting for different types of wrapped lists is not straightforward at the moment.

Khady

thanks :)

Signed-off-by: Nicolás Ojeda Bär <[email protected]> Signed-off-by: Marek Kubica <[email protected]>

Signed-off-by: Marek Kubica <[email protected]>

Leonidas-from-XIV · 2024-10-18T09:43:56Z

I added a changelog entry and merged it to move this a bit forward :-) @nojb feel free to adjust the wording if that's not what you had in mind.

nojb · 2024-10-18T11:03:22Z

@Leonidas-from-XIV Seems fine, thanks. The reason I had not merged it is that I was not 100% sure that the new style is always better than the old style, but I guess there's no harm to try it out. Thanks!

…g wrapped lists (ocaml#10892) * format-dune-file: use a hov box instead of a hv box for wrapped lists Signed-off-by: Nicolás Ojeda Bär <[email protected]> Signed-off-by: Marek Kubica <[email protected]> * Add test Signed-off-by: Nicolás Ojeda Bär <[email protected]> Signed-off-by: Marek Kubica <[email protected]> * Accept Signed-off-by: Nicolás Ojeda Bär <[email protected]> Signed-off-by: Marek Kubica <[email protected]> * Add changelog entry Signed-off-by: Marek Kubica <[email protected]> --------- Signed-off-by: Nicolás Ojeda Bär <[email protected]> Signed-off-by: Marek Kubica <[email protected]> Co-authored-by: Marek Kubica <[email protected]>

@jchavarri

CHANGES: ### Fixed - Show the context name for errors happening in non-default contexts. (ocaml/dune#10414, fixes ocaml/dune#10378, @jchavarri) - Correctly declare dependencies of indexes so that they are rebuilt when needed. (ocaml/dune#10623, @voodoos) - Don't depend on coq-stdlib being installed when expanding variables of the `coq.version` family (ocaml/dune#10631, fixes ocaml/dune#10629, @gares) - Error out if no files are found when using `copy_files`. (ocaml/dune#10649, @jchavarri) - Re_export dune-section private library in the dune-site library stanza, in order to avoid failure when generating and building sites modules with implicit_transitive_deps = false. (ocaml/dune#10650, fixes ocaml/dune#9661, @MA0100) - Expect test fixes: support multiple modes and fix dependencies when there is a custom runner (ocaml/dune#10671, @vouillon) - In a `(library)` stanza with `(extra_objects)` and `(foreign_stubs)`, avoid double linking the extra object files in the final executable. (ocaml/dune#10783, fixes ocaml/dune#10785, @nojb) - Map `(re_export)` library dependencies to the `exports` field in `META` files, and vice-versa. This field was proposed in to https://discuss.ocaml.org/t/proposal-a-new-exports-field-in-findlib-meta-files/13947. The field is included in Dune-generated `META` files only when the Dune lang version is >= 3.17. (ocaml/dune#10831, fixes ocaml/dune#10830, @nojb) - Fix staged pps preprocessors on Windows (which were not working at all previously) (ocaml/dune#10869, fixes ocaml/dune#10867, @nojb) - Fix `dune describe` when an executable is disabled with `enabled_if`. (ocaml/dune#10881, fixes ocaml/dune#10779, @moyodiallo) - Fix an issue where C stubs would be rebuilt whenever the stderr of Dune was redirected. (ocaml/dune#10883, fixes ocaml/dune#10882, @nojb) - Format long lists in s-expressions to fill the line instead of formatting them in a vertical way (ocaml/dune#10892, fixes ocaml/dune#10860, @nojb) - Fix the URL opened by the command `dune ocaml doc`. (ocaml/dune#10897, @gridbugs) - Fix the file referred to in the error/warning message displayed due to the dune configuration version not supporting a particular configuration stanza in use. (ocaml/dune#10923, @H-ANSEN) - Fix `enabled_if` when it uses `env` variable. (ocaml/dune#10936, fixes ocaml/dune#10905, @moyodiallo) - Fix exec -w for relative paths with --root argument (ocaml/dune#10982, @gridbugs) - Do not ignore the `(locks ..)` field in the `test` and `tests` stanza (ocaml/dune#11081, @rgrinberg) - Tolerate files without extension when generating merlin rules. (ocaml/dune#11128, @anmonteiro) ### Added - Make Merlin/OCaml-LSP aware of "hidden" dependencies used by `(implicit_transitive_deps false)` via the `-H` compiler flag. (ocaml/dune#10535, @voodoos) - Add support for the -H flag (introduced in OCaml compiler 5.2) in dune (requires lang versions 3.17). This adaptation gives the correct semantics for `(implicit_transitive_deps false)`. (ocaml/dune#10644, fixes ocaml/dune#9333, ocsigen/tyxml#274, ocaml/dune#2733, ocaml/dune#4963, @MA0100) - Add support for specifying Gitlab organization repositories in `source` stanzas (ocaml/dune#10766, fixes ocaml/dune#6723, @H-ANSEN) - New option to control jsoo sourcemap generation in env and executable stanza (ocaml/dune#10777, fixes ocaml/dune#10673, @hhugo) - One can now control jsoo compilation_mode inside an executable stanza (ocaml/dune#10777, fixes ocaml/dune#10673, @hhugo) - Add support for specifying default values of the `authors`, `maintainers`, and `license` stanzas of the `dune-project` file via the dune config file. Default values are set using the `(project_defaults)` stanza (ocaml/dune#10835, @H-ANSEN) - Add names to source tree events in performance traces (ocaml/dune#10884, @jchavarri) - Add `codeberg` as an option for defining project sources in dune-project files. For example, `(source (codeberg user/repo))`. (ocaml/dune#10904, @nlordell) - `dune runtest` can now run individual tests with `dune runtest mytest.t` (ocaml/dune#11041, @Alizter). - Wasm_of_ocaml support (ocaml/dune#11093, @vouillon) - Add a `coqdep_flags` field to the `coq` field of the `env` stanza, and to the `coq.theory` stanza, allowing to configure `coqdep` flags. (ocaml/dune#11094, @rlepigre) ### Changed - Remove all remnants of the experimental `patch-back-source-tree`. (ocaml/dune#10771, @rgrinberg) - Change the preset value for author and maintainer fields in the `dune-project` file to encourage including emails. (ocaml/dune#10848, @punchagan) - Tweak the preset value for tags in the `dune-project` file to hint at topics not having a special meaning. (ocaml/dune#10849, @punchagan) - Change some colors to improve readability in light-mode terminals (ocaml/dune#10890, @gridbugs) - Forward the linkall flag to jsoo in whole program compilation as well (ocaml/dune#10935, @hhugo) - Configurator uses `pkgconf` as pkg-config implementation when available and forwards it the `target` of `ocamlc -config`. (ocaml/dune#10937, @pirbo) - Enable Dune cache by default. Add a new Dune cache setting `enabled-except-user-rules`, which enables the Dune cache, but excludes user-written rules from it. This is a conservative choice that can avoid breaking rules whose dependencies are not correctly specified. This is the current default. (ocaml/dune#10944, ocaml/dune#10710, @nojb, @ElectreAAS) - Do not add `dune` dependency in `dune-project` when creating projects with `dune init proj`. The Dune dependency is implicitely added when generating opam files (ocaml/dune#11129, @Leonidas-from-XIV)

maiste · 2024-11-27T10:40:40Z

While releasing 3.17 we found that this PR seems to break odoc @runtest (see the CI). @nojb is the version here related to the dune version or the version from the lang dune <version> in dune-project? If this is the executable version, it would break dune for people doing some tests with sexpr as long as they upgrade to dune.3.17.0.

nojb · 2024-11-27T12:22:00Z

While releasing 3.17 we found that this PR seems to break odoc @runtest (see the CI). @nojb is the version here related to the dune version or the version from the lang dune <version> in dune-project? If this is the executable version, it would break dune for people doing some tests with sexpr as long as they upgrade to dune.3.17.0.

I'm away from my computer at the moment. I'll take a look later and post back here any findings.

nojb · 2024-11-27T12:33:21Z

While releasing 3.17 we found that this PR seems to break odoc @runtest (see the CI). @nojb is the version here related to the dune version or the version from the lang dune <version> in dune-project? If this is the executable version, it would break dune for people doing some tests with sexpr as long as they upgrade to dune.3.17.0.

I'm away from my computer at the moment. I'll take a look later and post back here any findings.

From a quick glance: the rule in question

https://github.com/ocaml/odoc/blob/822d266232fccdffbd4922434c81c45ab6d583f4/test/generators/dune#L11-L16

uses dune format-dune-file without a --dune-version argument, so the latest available version of the Dune language will be used, in which the case the observed behaviour is the expected one.

maiste · 2024-11-27T13:42:50Z

Thanks for having taken the time to look at it! 🙏

Is this behaviour specific to dune format-dune-file where you have to specify the version or it will use the executable version?

nojb · 2024-11-27T14:06:38Z

Is this behaviour specific to dune format-dune-file where you have to specify the version or it will use the executable version?

Yes, it is specific to this command, which does not assume to be in a Dune workspace at all to begin with.

Leonidas-from-XIV · 2024-11-27T14:09:55Z

Thanks for the investigation, you're right that format-dune-file has this documented:

       --dune-version=VERSION (absent=3.17)
           Which version of Dune language to use.

I personally would argue that this is quite unhelpful default as then the editor needs to parse the dune lang version or end up with formatting that differs from what dune fmt will do in the same project.

It's also different from how the rest of Dune works.

nojb · 2024-11-27T14:14:03Z

It's also different from how the rest of Dune works.

Not really, as there are no analogs of this command in Dune: Dune is not typically supposed to call Dune in a user rule, and indeed dune format-dune-file was mostly added as a debugging aid, not meant to be used from user rules. Instead, we could/should expose a new type of action (ie (format-dune-file <path>)) that could be used in user rules and that would take into account the current version of the Dune language.

I can look into adding this, but it won't fix the immediate issue, of course.

maiste · 2024-11-27T14:25:34Z

@nojb would it be a problem if we remove the fix from 3.17.0 and reintroduce it in a future dune release? It would give us more time to "fix" odoc and make them ready to support this change. WDYT?

nojb · 2024-11-27T14:28:51Z

@nojb would it be a problem if we remove the fix from 3.17.0 and reintroduce it in a future dune release? It would give us more time to "fix" odoc and make them ready to support this change. WDYT?

Sounds good to me.

Leonidas-from-XIV · 2024-11-27T14:37:42Z

The documentation states that this is also supposed to be used for editors, but if an editor does not use the --dune-version command, then it will still format the files in an inconsistent way.

And while I can see the point of being independent from the workspace, I would expect that in the presence of the workspace there would be a way to use the dune-lang version, without having to parse dune-project manually.

The reason why I think it is wrong is because I am thinking what what PR to send upstream to odoc to fix this:

I can add --dune-version=3.16 to keep it at a fixed version, but this will break the lower-bounds check if OPAM selects Dune 3.7 (their current dune-lang version)
I can specify --dune-version=3.7 but this is fairly ugly because now odoc needs to update two places. I thought that maybe --dune-version=%{dune_lang_version} could be used, but such a variable does not exist.
We could introduce --read-dune-version-from-workspace but I really don't like flags of the --do-the-right-thing kind as people need to be aware of them so it pushes the duty to do the right thing onto users.
We could, as you suggest expose an action, but this doesn't help editors, so if they use this command they still need to determine the dune-lang version to use it properly.

My suggestion would be skipping this from 3.17 and reintroducing this in 3.18 with a change that if --dune-version is not specified, it tries to read the version from the project and if that fails then it falls back to using the dune-version. WDYT?

nojb · 2024-11-27T14:40:19Z

with a change that if --dune-version is not specified, it tries to read the version from the project and if that fails then it falls back to using the dune-version.

Sounds reasonable.

…ormatting wrapped lists (ocaml#10892)" This reverts commit af95981.

…ormatting wrapped lists (ocaml#10892)" This reverts commit af95981. Signed-off-by: Etienne Marais <[email protected]>

nojb · 2024-11-28T04:08:08Z

with a change that if --dune-version is not specified, it tries to read the version from the project and if that fails then it falls back to using the dune-version.

Sounds reasonable.

Thinking more about this: read the version from which project? The one corresponding to the current working directory, or the one containing the target Dune file? I guess the former, if we want to be uniform with other Dune invocations.

Leonidas-from-XIV · 2024-11-28T10:18:35Z

The one that the file would be belonging to, a little bit like vendoring a project creates a barrier for automatic formatting.

Khady · 2024-12-02T07:27:23Z

if dune format-dune-file starts to read some of the dune config for the project, could it go all the way and solve #3516?

nojb · 2024-12-02T08:24:54Z

if dune format-dune-file starts to read some of the dune config for the project, could it go all the way and solve #3516?

I guess so. I think it would be the same as calling dune build @fmt but restricting the effect to a single file. Needs to be checked that this behaviour is OK for existing users of dune format-dune-file. You mentioned OCaml-LSP in #3516. Are you aware of any others?

Incidentally, in #11166 there is a first (orthogonal) step which is to expose a (format-dune-file) action to be used within Dune files instead of calling dune format-dune-file.

Khady · 2024-12-04T02:12:15Z

Are you aware of any others?

Nothing that comes directly to my mind

nojb mentioned this pull request Sep 6, 2024

auto formatting of run shouldn't split its content into one word per line #10860

Closed

rgrinberg approved these changes Sep 6, 2024

View reviewed changes

Khady approved these changes Sep 10, 2024

View reviewed changes

nojb and others added 4 commits October 18, 2024 10:51

format-dune-file: use a hov box instead of a hv box for wrapped lists

c9bb4ad

Signed-off-by: Nicolás Ojeda Bär <[email protected]> Signed-off-by: Marek Kubica <[email protected]>

Add test

dff5283

Signed-off-by: Nicolás Ojeda Bär <[email protected]> Signed-off-by: Marek Kubica <[email protected]>

Accept

b1eaa6c

Signed-off-by: Nicolás Ojeda Bär <[email protected]> Signed-off-by: Marek Kubica <[email protected]>

Add changelog entry

0de5113

Signed-off-by: Marek Kubica <[email protected]>

Leonidas-from-XIV force-pushed the tweak_dune_file_formatting branch from 5158629 to 0de5113 Compare October 18, 2024 08:51

Leonidas-from-XIV merged commit af95981 into ocaml:main Oct 18, 2024
26 of 27 checks passed

nojb deleted the tweak_dune_file_formatting branch October 18, 2024 09:45

maiste mentioned this pull request Nov 25, 2024

[new release] dune (15 packages) (3.17.0~alpha0) ocaml/opam-repository#26951

Closed

maiste added a commit to maiste/dune that referenced this pull request Nov 27, 2024

Revert "Tweak dune file formatting: avoid pure vertical layout when f…

a0cdeaa

…ormatting wrapped lists (ocaml#10892)" This reverts commit af95981.

maiste mentioned this pull request Nov 27, 2024

[3.17] remove breaking change for alpha1 #11164

Merged

maiste added a commit to maiste/dune that referenced this pull request Nov 27, 2024

Revert "Tweak dune file formatting: avoid pure vertical layout when f…

7f41f4e

…ormatting wrapped lists (ocaml#10892)" This reverts commit af95981. Signed-off-by: Etienne Marais <[email protected]>

nojb mentioned this pull request Nov 28, 2024

Add (format-dune-file) action #11166

Merged

ElectreAAS mentioned this pull request Nov 29, 2024

3.17.0 release tracking #11143

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tweak dune file formatting: avoid pure vertical layout when formatting wrapped lists #10892

Tweak dune file formatting: avoid pure vertical layout when formatting wrapped lists #10892

nojb commented Sep 6, 2024 •

edited

Loading

rgrinberg left a comment

Khady commented Sep 7, 2024

nojb commented Sep 7, 2024

Khady left a comment

Leonidas-from-XIV commented Oct 18, 2024

nojb commented Oct 18, 2024

maiste commented Nov 27, 2024 •

edited

Loading

nojb commented Nov 27, 2024

nojb commented Nov 27, 2024

maiste commented Nov 27, 2024 •

edited

Loading

nojb commented Nov 27, 2024

Leonidas-from-XIV commented Nov 27, 2024 •

edited

Loading

nojb commented Nov 27, 2024 •

edited

Loading

maiste commented Nov 27, 2024

nojb commented Nov 27, 2024

Leonidas-from-XIV commented Nov 27, 2024 •

edited

Loading

nojb commented Nov 27, 2024

nojb commented Nov 28, 2024 •

edited

Loading

Leonidas-from-XIV commented Nov 28, 2024

Khady commented Dec 2, 2024

nojb commented Dec 2, 2024

Khady commented Dec 4, 2024

Tweak dune file formatting: avoid pure vertical layout when formatting wrapped lists #10892

Tweak dune file formatting: avoid pure vertical layout when formatting wrapped lists #10892

Conversation

nojb commented Sep 6, 2024 • edited Loading

rgrinberg left a comment

Choose a reason for hiding this comment

Khady commented Sep 7, 2024

nojb commented Sep 7, 2024

Khady left a comment

Choose a reason for hiding this comment

Leonidas-from-XIV commented Oct 18, 2024

nojb commented Oct 18, 2024

maiste commented Nov 27, 2024 • edited Loading

nojb commented Nov 27, 2024

nojb commented Nov 27, 2024

maiste commented Nov 27, 2024 • edited Loading

nojb commented Nov 27, 2024

Leonidas-from-XIV commented Nov 27, 2024 • edited Loading

nojb commented Nov 27, 2024 • edited Loading

maiste commented Nov 27, 2024

nojb commented Nov 27, 2024

Leonidas-from-XIV commented Nov 27, 2024 • edited Loading

nojb commented Nov 27, 2024

nojb commented Nov 28, 2024 • edited Loading

Leonidas-from-XIV commented Nov 28, 2024

Khady commented Dec 2, 2024

nojb commented Dec 2, 2024

Khady commented Dec 4, 2024

nojb commented Sep 6, 2024 •

edited

Loading

maiste commented Nov 27, 2024 •

edited

Loading

maiste commented Nov 27, 2024 •

edited

Loading

Leonidas-from-XIV commented Nov 27, 2024 •

edited

Loading

nojb commented Nov 27, 2024 •

edited

Loading

Leonidas-from-XIV commented Nov 27, 2024 •

edited

Loading

nojb commented Nov 28, 2024 •

edited

Loading