Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Pass documentation along with signatures #627

Merged
merged 11 commits into from
Mar 10, 2021

Conversation

Julow
Copy link
Collaborator

@Julow Julow commented Mar 9, 2021

Before this PR, the top-comment of a signature was handled unevenly and sometimes dropped.
This PR improves how the top-comment is passed and implement @lpw25's suggestion.

An issue remain in #478, the documentation from an @inline include is not in the preamble and is not used for the synopsis due to being handled too late. This will be fixed in a follow-up PR.

@Julow Julow requested a review from jonludlam March 9, 2021 18:35
Copy link
Member

@jonludlam jonludlam left a comment

Choose a reason for hiding this comment

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

Looking good!

|| List.length s.module_type_of_invalidating_modules > 0
then false
else compiled
let really_compiled =
Copy link
Member

Choose a reason for hiding this comment

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

needs_recompilation might be a better name here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I went for dont_recompile to keep the compiled = t.compiled && dont_recompile pattern.

@jonludlam
Copy link
Member

There are test failures in <=4.06 though, which will need addressing.

Julow added 9 commits March 10, 2021 15:27
This field correspond to the top comment of the signature, which is
removed from the 'items' list.
This reverts commit d53ba9f.

This is no longer necessary since we don't loose the top comments
anymore (see parent).
Handle the 'doc' field of signatures in the generator.

Doc attached to a definition is no longer rendered in its expansion.
This will be fixed the next commits.
If there is no documentation attached to the declaration, use the
expansion signature's doc
(follow recursively in case of functors, etc...).

This doesn't apply to module aliases.
Like module signatures, extract the top-comment from the loader and pass
it through. Render it the same way as modules.
@Julow Julow force-pushed the top-comment-handling branch from cc2445a to 910995d Compare March 10, 2021 14:28
@Julow Julow force-pushed the top-comment-handling branch from 910995d to dadc5cc Compare March 10, 2021 14:33
@Julow
Copy link
Collaborator Author

Julow commented Mar 10, 2021

The tests failures were caused by the comment not represented by a floating attribute with OCaml <= 4.06 in

module M : sig
  (** Doc. *)
end

however it is in

module M : sig

  (** Doc. *)
end

It is present in cmti files but I couldn't find a way to retrieve it, any idea ?

@jonludlam
Copy link
Member

jonludlam commented Mar 10, 2021

Testing with 4.06.1, using current master odoc on

module M : sig
  (** Doc. *)
end

I don't get the documentation in module M, so it's not a regression. Also, I get

File "m.mli", line 2, characters 8-18:
Error (warning 50): unattached documentation comment (ignored)

@jonludlam jonludlam merged commit f26a115 into ocaml:master Mar 10, 2021
@Julow
Copy link
Collaborator Author

Julow commented Mar 10, 2021

Indeed not a regression. I missed the warning.

@@ -27,21 +27,18 @@ <h1>
<p>
Some additional comments.
</p>
<h2 id="type">
<a href="#type" class="anchor"></a>Type
</h2>
Copy link
Contributor

Choose a reason for hiding this comment

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

This merge broke the HTML structure. This h2 has nothing to do here, it should still be in odoc-content.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Issue: #631

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants