Add module indent option

[HPRIOR]

Adds indent option for module structs, types and signatures.

[EmileTrotignon]

Thanks for the PR !

The implementation looks very clean, I don't think this will cause a maintenance burden in the future.

I think the option should be named struct-indent, and the documentation should make clear that this affects the inside of sig ... end and struct ... end blocks, currently the explanation is not precise enough. I know struct-indent feels a bit weird because it also affects sig, but there is precedent with break-struct also affecting sig.

The tests could include attributes and comments to check that everything works well when they are present.

dune build @gen_manpage --auto-promote should be run to regenerate the manpage.

There also need to be an entry in CHANGES.md.
The CI will stay red because test-branch is currently broken, but the Build on Linux should become green once the manpage is up to date.

[Julow]

I know struct-indent feels a bit weird because it also affects sig, but there is precedent with break-struct also affecting sig.

I don't think struct-indent is a good name. I understand that module in the option name means struct .. end and sig .. end and I don't think can be misunderstood for other module expressions. struct seems to explicitly exclude signatures, when the option doesn't from its conception.
The way to go for consistency, I think, is to rename the other options instead.

The CI will stay red because test-branch is currently broken

test-branch shouldn't fail the CI, even when there are errors. It should be fixed because it's an extremely important tool.

[Julow]

OCamlformat has been widely used for many years, and I've never seen this option being requested until last month (#2677).
We generally don't want to add options to OCamlformat because it makes the code less consistent and harder to read across the community. It also creates an exponentially increasing number of possible configurations that we can't test and that contain countless bugs.

The implementation looks good and I won't object to merging. I wonder what the community feels about this PR ?

[EmileTrotignon]

It was requested in the past here: #1613

My opinion on introducing options is that it really depends on the option. This one is very simple, should not cause maintenance issues nor bugs.

That is not the case for other options that have a complex implementation and a hard to explain behavior (the worst one of course being ocp-indent-compat, but there are others in between).

[EmileTrotignon]

As for the name, in the discussions leading up the PR, the question of this option affecting functor applications came up, so its at least a little confusing. Also, module excludes sig, because sig .. end is the syntax for module types.

What about struct-sig-indent ?

[HPRIOR]

Thank you for reviewing @Julow.

We generally don't want to add options to OCamlFormat because it makes the code less consistent and harder to read across the community. It also creates an exponentially increasing number of possible configurations that we can't test and that contain countless bugs.

I understand the concern about adding too many options. I think my PR does contribute to this problem. There are several syntax elements that I’ve left out of the implementation because it’s unclear whether they should follow the module-indent setting.

For example, extension nodes:

 module M =
-  [%demo
-  module Foo = Bar
+    [%demo
+    module Foo = Bar

-  type t]
+    type t]

And anonymous modules:

 module Subst = Map.Make (struct
-    type t = string
+        type t = string

-    let compare = compare
-  end)
+        let compare = compare
+    end)

If these aren't covered by module-indent, we’d need separate options like module-extension-node-indent or anon-module-indent, which would add to the option/maintenance bloat.

@Julow would you be supportive of a global indent option instead? Without being complemented by additional options, the current implementation is insufficient for my needs since I'd like all module items to be consistently indented. @EmileTrotignon mentioned in the discussion here #2677 (comment) that there may be some hidden edge cases when increasing the indentation levels above the default. Given the formatter already supports wrapping/breaking at arbitrary widths it seems like the existing logic is capable of supporting the feature otherwise. While it may slightly increase the developement cost where edge cases are found by the minority of users using non-default indentation, it would not cause configuration/option bloat.

I don't think it's worth arguing for the merits of different indentation levels. What matters is that people have strong preferences, and there’s currently no way to reflect them in ocaml. As @ELLIOTTCABLE argued in #1613, even very opinionated formatters like Prettier allow users to choose indentation width. The widespread use of the default isn’t surprising when there’s never been a choice.

[HPRIOR]

Thanks for reviewing @EmileTrotignon,

The tests could include attributes and comments to check that everything works well when they are present.

dune build @gen_manpage --auto-promote should be run to regenerate the manpage.

There also need to be an entry in CHANGES.md. The CI will stay red because test-branch is currently broken, but the Build on Linux should become green once the manpage is up to date.

If I decide to proceed with the PR I'll make these changes, otherwise it's useful to know for the next one.

[EmileTrotignon]

While it may slightly increase the developement cost where edge cases are found by the minority of users using non-default indentation, it would not cause configuration/option bloat.

In my opinion, such a widespread configuration option would be way harder to merge than the current, or even a slightly extented version of the current PR. The reason being that this PR has a defined scope which is very important for a bug-free implementation. To me, the biggest issue with option bloat is the implementation. In this perspective, a small, clearly bugfree PR like is easy to merge, but a PR with bigger changes not so much.

That being said, I understand the appeal of the feature. A plan to get it would be to have more small indentation options
in the future, and a master option to set them all at once (the logic for that already exists in the option for ocp-indent configuration).