Add the doc-comments-val option by Julow · Pull Request #1012 · ocaml-ppx/ocamlformat

Julow · 2019-09-10T16:16:56Z

Partial fix to #1010

~~Add doc-comments=after-val that is like after for vals and externals and like before for other items.~~
Edit: This PR adds doc-comments-val=before|after to control placement of doc comments on val and external only.

This removes the inconsistency caused by the is_simple heuristic on modules and classes or by the attachments of doc comments on constructor by OCaml's parser:

(** This comment goes before *)
module S_ext : sig
  type t
end

module Index : Index.S
(** This one goes after *)

type t = int
(** t *)

(** t' *)
type t' = Foo

This is added to the conventional profile.

craigfe · 2019-09-11T09:47:50Z

Adding my (underqualified) opinions given offline:

I think this is a good short-term solution for backwards compatibility reasons, but that after should be deprecated as a confusing option.
I still think the most intuitive interface would be something like
- doc-comments-val={before,_after_}
- doc-comments-struct={_before_,after,auto}
  potentially with the 'auto' option being deprecated depending on how much people care about this heuristic. If the heuristic is to be kept, it could benefit from improvements re. functors as suggested in Style suggestion: consistent placing of structure item doc comments #1010.

Also worth noting that this does nothing to resolve the variants issue highlighted by @Julow here, since that's an OCaml parser problem.

gpetiot · 2019-09-12T04:10:45Z

test/passing/doc_comments-after_val.ml.ref

+      inherit a
+
+      (** Val *)
+      val x = 1


should be after?

This is intended, after-val is about signatures and not implementations.
However, placing comments after vals and methods in class types would make sense.

I implemented that on every class type fields, including inherit and constraint, since they should always be on a single line. What do you think ?

gpetiot · 2019-09-12T04:13:35Z

Having 2 options makes sense to me, to avoid having weird option values like after-val that would be confusing to the users.

I have a question though, why is this new behavior applied to val/external in signatures but not to the type definitions or other signature elements? What makes the val so specific?

It seems like it is only applied for toplevel/module values, and not for class values, is this intended?

Julow · 2019-09-12T11:42:30Z

The current doc-comments=after option puts the comments after declarations except:

Variant declarations, otherwise that would change the AST
"not simple" modules
"not simple" classes

This new value removes these special cases while still placing comments after val and external declarations in signatures.

Julow · 2019-10-16T13:59:58Z

What should we do with this ?
I don't like the idea of adding an other option that would only be used when an other option is set to some value.
I think that adding after-val solves the problem and keep the power of that option low.

gpetiot · 2019-10-16T14:51:16Z

I'm just not convinced with the name, maybe auto would be better, since we basically decide what's better instead of the user, depending of the size and kind of the item.

Julow · 2019-10-17T12:58:40Z

I don't like auto because the rules are simple and the user will know exactly how it will format.
Do you mean renaming after to auto ? This would make sense, yes.

Julow · 2019-10-17T13:11:02Z

Just rebased.

gpetiot · 2019-10-17T15:07:26Z

If replacing after by auto makes sense, why not.
But to me after-val sounds way too close to after and yet very specific, do after and after-val really need to co-exist? Would it be feasible to keep only one of them?
I'm more reluctant to add more options/values now that we experienced how difficult it is to remove/modify it when it's released out there.

Julow · 2019-10-18T12:27:55Z

The reason for this new value is because after has too arbitrary and hard to understand. Users can't predict where the docstring will be placed and just assume this is a bug.
after-val has a much simpler rule, it places docstrings after val/external declarations and before everything else.
I agree the name is not nice and looks like a special case.

An alternative would be to deprecate doc-comments, making it default to before and adding a new doc-comments-val=before|after to control just that.

gpetiot · 2019-10-18T15:51:12Z

An alternative would be to deprecate doc-comments, making it default to before and adding a new doc-comments-val=before|after to control just that.

This sounds better indeed, as long as there is no regression for the janestreet profile, and only improvements for the conventional and ocamlformat profiles I'm all for it.

Julow · 2019-10-21T15:13:06Z

I changed to keep doc-comments as-is and added doc-comments-val=before|after.
In conventional it's set as doc-comments=before and doc-comments-val=after.

Only the ocamlformat profile uses doc-comments=after.
Should we deprecate doc-comments ?
If yes, should we set change the ocamlformat profile to allow to remove that option ?

gpetiot · 2019-10-21T16:47:24Z

Only the ocamlformat profile uses doc-comments=after.
Should we deprecate doc-comments ?
If yes, should we set change the ocamlformat profile to allow to remove that option ?

How does the diff look like? Docstrings for big elements should already be before.

Julow · 2019-10-22T08:53:25Z

The diff is huge on infer, mainly because of doc comments on types.
A more reasonable plan would be to deprecate after to later rename it to something like auto. What do you think?

gpetiot · 2020-02-17T14:28:47Z

We can avoid going through a deprecation step if doc-comments is in the way of having a consistent doc-comments-val, so to summarize we will have:

doc-comments-val={before,after}
doc-comments is removed and its default will be the current before

but printing a custom message when this option is parsed would be useful for the user

Julow · 2020-02-17T16:04:30Z

I rebased and cleaned the history a bit. There quite a bit of changes with test_branch but they all are expected.
I'm Ok with removing doc-comments without warning. We should do it in an other PR, as we may want to log that properly.

gpetiot

It looks much better now, thanks. ocamlformat-help.txt has not been completely updated though (with the new doc-comments doc).

lib/Fmt_ast.ml

@attr

CHANGES: #### New features + Add an option `--format-invalid-files` to print unparsable parts of the input as verbatim text. This feature is still experimental. (ocaml-ppx/ocamlformat#1026) (Guillaume Petiot) + Support multi-indices extended indexing operators (ocaml-ppx/ocamlformat#1279, ocaml-ppx/ocamlformat#1277) (Jules Aguillon, Guillaume Petiot) This feature has been added in OCaml 4.10.0 + Handle OCaml 4.10.0 AST (ocaml-ppx/ocamlformat#1276) (Guillaume Petiot) + Preserve functor syntax for consistency (ocaml-ppx/ocamlformat#1312) (Guillaume Petiot) Previously both functor syntax: `module M = functor (K : S) -> struct end` and `module M (K : S) = struct end` would be formatted as the latter, the original syntax is now preserved. #### Changes + Add the option `doc-comments-val=before|after` (ocaml-ppx/ocamlformat#1012) (Jules Aguillon) This option set the placement of documentation comment on `val` and `external` only. It is set to `after` by default. + The default for `doc-comments` is changed from `after` to `before` (ocaml-ppx/ocamlformat#1012, ocaml-ppx/ocamlformat#1325) (Jules Aguillon) This affects both `conventional` (default) and `ocamlformat` profiles. + Some options are now deprecated: * `doc-comments` (ocaml-ppx/ocamlformat#1293, ocaml-ppx/ocamlformat#1012) This option depends on a flawed heuristic. It is replaced by `doc-comments-val` for `val` and `external` declarations. There is no equivalent to this option in the general case. * `escape-chars`, `escape-strings` and `extension-sugar` (ocaml-ppx/ocamlformat#1293) These options are rarely used and their default behavior is considered to be the right behavior. + Add space between `row_field` attributes and the label or arguments, to be consistent with the non-polymorphic case. (ocaml-ppx/ocamlformat#1299) (Craig Ferguson) #### Bug fixes + Fix missing parentheses around `let open` (ocaml-ppx/ocamlformat#1229) (Jules Aguillon) eg. `M.f (M.(x) [@attr])` would be formatted to `M.f M.(x) [@attr]`, which would crash OCamlformat + Remove unecessary parentheses with attributes in some structure items: * extensions and eval items (ocaml-ppx/ocamlformat#1230) (Jules Aguillon) eg. the expression `[%ext (() [@attr])]` or the structure item `(() [@attr]) ;;` * `let _ = ...` constructs (ocaml-ppx/ocamlformat#1244) (Etienne Millon) + Fix some bugs related to comments: * after a function on the rhs of an infix (ocaml-ppx/ocamlformat#1231) (Jules Aguillon) eg. the comment in `(x >>= fun y -> y (* A *))` would be dropped * in module unpack (ocaml-ppx/ocamlformat#1309) (Jules Aguillon) eg. in the module expression `module M = (val x : S (* A *))` + Fix formatting of empty signature payload `[%a:]` (ocaml-ppx/ocamlformat#1236) (Etienne Millon) + Fix parenthesizing when accessing field of construct application (ocaml-ppx/ocamlformat#1247) (Guillaume Petiot) + Fix formatting of attributes on object overrides `{< >}` (ocaml-ppx/ocamlformat#1238) (Etienne Millon) + Fix attributes on coercion (ocaml-ppx/ocamlformat#1239) (Etienne Millon) + Fix formatting of attributes on packed modules (ocaml-ppx/ocamlformat#1243) (Etienne Millon) + Fix parens around binop operations with attributes (ocaml-ppx/ocamlformat#1252, ocaml-ppx/ocamlformat#1306) (Guillaume Petiot, Craig Ferguson) + Remove unecessary parentheses in the argument of indexing operators (ocaml-ppx/ocamlformat#1280) (Jules Aguillon) + Retain attributes on various AST nodes: * field set expressions, e.g. `(a.x <- b) [@A]` (ocaml-ppx/ocamlformat#1284) (Craig Ferguson) * instance variable set expressions, e.g. `(a <- b) [@A]` (ocaml-ppx/ocamlformat#1288) (Craig Ferguson) * indexing operators, e.g. `(a.(b)) [@A]` (ocaml-ppx/ocamlformat#1300) (Craig Ferguson) * sequences, e.g. `(a; b) [@A]` (ocaml-ppx/ocamlformat#1291) (Craig Ferguson) + Avoid unnecessary spacing after object types inside records and polymorphic variants, e.g. `{foo : < .. > [@A]}` and `{ foo : < .. > }` (ocaml-ppx/ocamlformat#1296) (Craig Ferguson) + Fix missing parentheses around tuples with attributes. (ocaml-ppx/ocamlformat#1301) (Craig Ferguson) Previously, `f ((0, 0) [@A])` would be formatted to `f (0, 0) [@A]`, crashing OCamlformat. + Avoid emitting `>]` when an object type is contained in an extension point or attribute payload (ocaml-ppx/ocamlformat#1298) (Craig Ferguson) + Fix crash on the expression `(0).*(0)` (ocaml-ppx/ocamlformat#1304) (Jules Aguillon) It was formatting to `0.*(0)` which parses as an other expression. + Preserve empty doc-comments syntax. (ocaml-ppx/ocamlformat#1311) (Guillaume Petiot) Previously `(**)` would be formatted to `(***)`. + Do not crash when a comment contains just a newline (ocaml-ppx/ocamlformat#1290) (Etienne Millon) + Handle lazy patterns as arguments to `class` (ocaml-ppx/ocamlformat#1289) (Etienne Millon) + Preserve cinaps comments containing unparsable code (ocaml-ppx/ocamlformat#1303) (Jules Aguillon) Previously, OCamlformat would fallback to the "wrapping" logic, making the comment unreadable and crashing in some cases. + Fix normalization of attributes, fixing the docstrings in attributes (ocaml-ppx/ocamlformat#1314) (Guillaume Petiot) + Add missing parentheses around OR-patterns with attributes (ocaml-ppx/ocamlformat#1317) (Guillaume Petiot) + Fix spacing inside parens for symbols when the spacing was handled by the englobing exp (ocaml-ppx/ocamlformat#1316) (Guillaume Petiot) + Fix invalid (unparsable) docstrings (ocaml-ppx/ocamlformat#1315) (Guillaume Petiot) When parsing a comment raises an error in odoc, it is printed as-is. + Fix parenthesizing of optional arguments rebound to non-variables, e.g. `let f ?a:(A) = ()` rather than the unparsable `let f ?a:A = ()` (ocaml-ppx/ocamlformat#1305) (Craig Ferguson)

CHANGES: #### Changes + The default for `doc-comments` is changed to `after` (ocaml-ppx/ocamlformat#1335) (Jules Aguillon) This reverts a change introduced in 0.14.0 (ocaml-ppx/ocamlformat#1012). + Revert deprecation of the `doc-comments` option (ocaml-ppx/ocamlformat#1331) (Jules Aguillon) This reverts a change introduced in 0.14.0 (ocaml-ppx/ocamlformat#1293).

facebook-github-bot added the CLA Signed label Sep 10, 2019

Julow requested a review from gpetiot September 10, 2019 16:19

Julow force-pushed the doc-comments-after-val branch from 959aef8 to e429a80 Compare September 10, 2019 16:21

gpetiot reviewed Sep 12, 2019

View reviewed changes

Julow force-pushed the doc-comments-after-val branch from f11e2d1 to a3bb19c Compare October 17, 2019 13:04

Julow force-pushed the doc-comments-after-val branch from 58255d3 to 02a8d72 Compare October 21, 2019 15:08

Julow requested review from gpetiot and jberdine October 21, 2019 15:13

Julow changed the title ~~Add doc-comments=after-val~~ Add the doc-comments-val option Oct 21, 2019

Julow force-pushed the doc-comments-after-val branch from 02a8d72 to a985f9b Compare November 6, 2019 13:06

Julow added 3 commits February 17, 2020 15:54

Add option doc-comments-val=before|after

ac5ad13

Set doc-comments=before by default

79cc3ff

Update CHANGES.md

45d866b

Julow force-pushed the doc-comments-after-val branch from a985f9b to 45d866b Compare February 17, 2020 15:46

gpetiot approved these changes Feb 17, 2020

View reviewed changes

Update ocamlformat-help.txt

a6da78d

Julow commented Feb 17, 2020

View reviewed changes

lib/Fmt_ast.ml Show resolved Hide resolved

Julow force-pushed the doc-comments-after-val branch from 381053c to 2c58f42 Compare February 17, 2020 18:05

Improve the documentation wording

2c58f42

Julow merged commit 319aa07 into ocaml-ppx:master Feb 17, 2020

Julow mentioned this pull request Apr 2, 2020

[new release] ocamlformat (0.14.0) ocaml/opam-repository#16133

Merged

This was referenced Apr 14, 2020

Revert doc-comments change to the default profile #1335

Merged

[new release] ocamlformat (0.14.1) ocaml/opam-repository#16198

Merged

gpetiot mentioned this pull request May 11, 2020

[new release] ocamlformat (0.14.2) ocaml/opam-repository#16407

Merged

Conversation

Julow commented Sep 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

craigfe commented Sep 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gpetiot Sep 12, 2019

Choose a reason for hiding this comment

Uh oh!

Julow Sep 12, 2019

Choose a reason for hiding this comment

Uh oh!

Julow Sep 12, 2019

Choose a reason for hiding this comment

Uh oh!

gpetiot commented Sep 12, 2019

Uh oh!

Julow commented Sep 12, 2019

Uh oh!

Julow commented Oct 16, 2019

Uh oh!

gpetiot commented Oct 16, 2019

Uh oh!

Julow commented Oct 17, 2019

Uh oh!

Julow commented Oct 17, 2019

Uh oh!

gpetiot commented Oct 17, 2019

Uh oh!

Julow commented Oct 18, 2019

Uh oh!

gpetiot commented Oct 18, 2019

Uh oh!

Julow commented Oct 21, 2019

Uh oh!

gpetiot commented Oct 21, 2019

Uh oh!

Julow commented Oct 22, 2019

Uh oh!

gpetiot commented Feb 17, 2020

Uh oh!

Julow commented Feb 17, 2020

Uh oh!

gpetiot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Julow commented Sep 10, 2019 •

edited

Loading

craigfe commented Sep 11, 2019 •

edited

Loading