Skip to content

Implement doc-comments and doc-comments-tag-only for every items#746

Merged
gpetiot merged 17 commits intoocaml-ppx:masterfrom
Julow:doc-comments-placmt
May 14, 2019
Merged

Implement doc-comments and doc-comments-tag-only for every items#746
gpetiot merged 17 commits intoocaml-ppx:masterfrom
Julow:doc-comments-placmt

Conversation

@Julow
Copy link
Copy Markdown
Collaborator

@Julow Julow commented Apr 1, 2019

Fix #723

This PR implements doc-comments=before|after and doc-comments-tag-only=default|fit for every structure, signature and class items:

  • Vals and externals
  • Type declarations
  • Modules
  • Includes and opens
  • Exceptions
  • Classes

Previously, doc-comments=after was only implemented for val and external items
and doc-comments-tag-only=fit for include, open and one-line module items.

Also silently implement doc-comments-padding for tag-only comments.

Docstring are always placed before in some cases:

  • let bindings
  • Variant declarations
  • Module/class declarations that are not "simple"

@jberdine
Copy link
Copy Markdown
Collaborator

jberdine commented Apr 1, 2019

Note that it is intentional that code items that are often longer than their docstrings, such as type declarations, structures, signatures, etc. are placed after their docstrings while code items that are commonly shorter than their docstrings are placed before their docstrings. This possibility should be preserved. I don't know if only 3 choices (current, all before, all after) are sufficient, or if finer granularity is better.

@Julow Julow force-pushed the doc-comments-placmt branch from b485d9d to 50c0301 Compare April 9, 2019 13:29
@Julow Julow force-pushed the doc-comments-placmt branch from 50c0301 to bb15e97 Compare May 3, 2019 16:39
@Julow
Copy link
Copy Markdown
Collaborator Author

Julow commented May 3, 2019

I rebased this PR, after Guillaume's tidying.
I disabled doc-comments=after when the module expression or type is not simple (using Ast.module_*_is_simple).

Copy link
Copy Markdown
Collaborator

@gpetiot gpetiot left a comment

Choose a reason for hiding this comment

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

Some regressions to fix before merging (and don't forget to make fmt), but I agree with the idea.

src/Ast.ml Outdated
| _ -> false
end
(** Operations determining precedence and necessary parenthesization of
terms based on their super-terms. *)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

module_expr_is_simple should not return true for this kind of module, we need to recursively call this function for Pmod_apply and Pmod_constraint (maybe other cases too if it makes sense, I didn't try it)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I improved this. I added some arbitrary rules (eg. 2 or more with-constraints is not simple, extensions are not simple, etc..). What do you think ?

end

module type A = B (** @open *)
module type A = B (** @open *)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If this intended?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes. This is the doc_comments_padding option. It was only used on type declarations until now.
@jberdine What do you think of this behavior ?

@gpetiot gpetiot force-pushed the doc-comments-placmt branch from 933dc52 to 0e1d621 Compare May 8, 2019 03:53
Copy link
Copy Markdown
Collaborator

@gpetiot gpetiot left a comment

Choose a reason for hiding this comment

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

I rebased it on master, minor comments about the simplicity of some items but otherwise looks good (no bug with test_branch).

-> Normalize.docstring_error list
; normalize: Conf.t -> 'a with_comments -> 'a
; printast: Caml.Format.formatter -> 'a -> unit }
(** Operations on translation units. *)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Maybe we need to add a Ast.type_is_simple for cases like this?

@Julow Julow force-pushed the doc-comments-placmt branch from 0e1d621 to a17b240 Compare May 9, 2019 15:00
@Julow
Copy link
Copy Markdown
Collaborator Author

Julow commented May 9, 2019

I implemented is_simple for class expressions and types.
Not for types yet, this would be a regression.

@gpetiot
Copy link
Copy Markdown
Collaborator

gpetiot commented May 13, 2019

I implemented is_simple for class expressions and types.
Not for types yet, this would be a regression.

The output looks better.

We can merge as soon as you took care of:

TODO:

Clean up docstring handling, especially for floating documentation

@Julow
Copy link
Copy Markdown
Collaborator Author

Julow commented May 13, 2019

I cleaned up the code. This is ready for review.

@Julow Julow changed the title [RFC] Implement doc-comments and doc-comments-tag-only for every items Implement doc-comments and doc-comments-tag-only for every items May 13, 2019
@Julow Julow requested a review from jberdine May 13, 2019 16:33
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.

Doc comment for modules do not follow --doc-comments option

4 participants