Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Comment attached to include are dropped #645

Closed
Julow opened this issue Mar 17, 2021 · 13 comments · Fixed by #647
Closed

Comment attached to include are dropped #645

Julow opened this issue Mar 17, 2021 · 13 comments · Fixed by #647
Milestone

Comments

@Julow
Copy link
Collaborator

Julow commented Mar 17, 2021

(** [T] decl comment. *)
module type T = sig
  (** [T] top-comment. *)

  type t
end

module M : sig
  (** Attached to [include]. *)
  include T
end

In the example above, the page for M renders like this:

image

The comment attached to the include is not rendered and T's top-comment is placed before the include T folding block.

I'd expect that T's top-comment is moved inside the folding block and the comment attached to the include placed before it:

> include T

Attached to include.

T top-comment

type t
@jonludlam
Copy link
Member

relevant: #87

@Julow
Copy link
Collaborator Author

Julow commented Mar 17, 2021

relevant: #87

This is covered. In my example, T's "decl comment" won't appear in the page for M but only in the page for T.

@jonludlam
Copy link
Member

These rules need to go into the doc that you've just PRd too!

@dbuenzli
Copy link
Contributor

I'd expect that T's top-comment is moved inside the folding block and the comment attached to the include placed before it:

As was mentioned in #612 it won't be possible to place T's include comment after the include declaration (unless you want it hidden when the details is closed but I don't think that's the idea). That would have been natural but we can't put it in the summary because of the content model and putting it after the details will push it after the include when the details is open which is not what you want.

Also I don't think it's a good idea to make a distinction between the "decl comment" and the "top comment". As far as rendering module M is concerned the following is the same to end-users:

(** Hey ho. 
      
      Bli bla. 
module M : sig
end
(** Hey ho. *)
module M : sig
    (** Bli bla *)
end
module M : sig 
   (** Hey ho 

         Bli bla *)
end 

and I don't expect users to make a difference between them.

Now if you start making differences with what is in the top comment and the declaration comment only when you include it will be totally unpredictable to the includer what she will get, because she's not the one who will have written M; she may even only see the docs of M, not the sources. So it the results will just be puzzling.

@dbuenzli
Copy link
Contributor

dbuenzli commented Mar 17, 2021

So if we want to follow what @lpw25 wants in #87 (I'm personally not entirely convinced) then what we should drop is the synopsis. Here are few possibilities to ponder:

  1. Always drop the synopsis of the includee.
  2. Drop the synopsis of the includee only if there's no there is a docstring for the include. Otherwise use the synopsis of the includee for the include's docstring.
  3. Never drop anything.

I'd be tempted by 2.

@Julow
Copy link
Collaborator Author

Julow commented Mar 18, 2021

Also I don't think it's a good idea to make a distinction between the "decl comment" and the "top comment". As far as rendering module M is concerned the following is the same to end-users:

This is what happens for modules but not what happens with module types:

(** Bli *)
module type T = sig (** Bla *) end

(** Blu *)
module M : T

T's preamble is Bli + Bla, M's preamble is Blu + Bla. There's discussion about this in #478
That's unrelated to this issue, in my example the comment in M is attached to the include.

The problem with documentation attached to includes, is that it would be lost because no page is generated for them.
This is fine for the top-comment of the includee, which is available through a link (except in case of @inline includes, in which case it's lost too).

I agree with your 2 too. It allows hiding the includee top-comment in case it's unwanted (for example if it says "this signature does ...") but still do the synopsis logic by default, consistently to other items.

As was mentioned in #612 it won't be possible to place T's include comment after the include declaration (unless you want it hidden when the details is closed but I don't think that's the idea).

I definitely expect it hidden when the details is closed. But more importantly, I think it should be placed after the "include" block for consistency. Currently, I see this comment has a floating comment and that can be confusing if the doc itself is vague about where it belongs (eg. the doc doesn't mention "include" because it assumes it'll be placed consistently).

The HTML backend should be able to not hide the comment when the details are closed, even if it's placed after. I'm not sure about other backends.

@dbuenzli
Copy link
Contributor

dbuenzli commented Mar 18, 2021

(** Bli *)
module type T = sig (** Bla *) end

(** Blu *)
module M : T

T's preamble is Bli + Bla, M's preamble is Blu + Bla.

On that example I agree, but for me, for the reasons mentioned above, it should not be about topdoc vs decl doc.

Nothing should ever be defined in these terms because the users of the declarations can't know how these were produced and the person who likely wrote them one of the other out of taste.

This means that if one writes:

(** Bli 

      Bla *)
module type T

(** Blu *)
module type M : T

M's preamble should still be Blu + Bla

It should always only be about playing with the synopsis. Nothing should ever be defined in terms this was in the topdoc or in the decldoc.

I definitely expect it hidden when the details is closed. But more importantly, I think it should be placed after the "include" block for consistency.

Personally docstring of includes only became news to me recently, but I think the idea is that you get a short description of what to expect if you click the disclosure button. So you don't want them hidden when the details is closed (and while inconsistent it's fine to have them before for usability reasons).

@Julow
Copy link
Collaborator Author

Julow commented Mar 18, 2021

Nothing should ever be defined in these terms because the users of the declarations can't know how these were produced and the person who likely wrote them one of the other out of taste.
[...]
M's preamble should still be Blu + Bla

Absolutely not. The top-comment is handled separately to the decl comment until the last stage where it is used to compose the preamble (and the synopsis). This is done on purpose, see #478 (comment)
This is transparent and work like you expect for each declarations but the top-comment is also propagated when using module types. module M : T is unrelated to the declaration of T but it "imports" everything inside the signature defined by T, even the top-comment.

About the current issue, does anyone have an opinion ?

@lpw25
Copy link
Contributor

lpw25 commented Mar 18, 2021

I think that we should always drop the synopsis of the includee. I don't think there is a need to distinguish whether the synopsis has come from the module or the signature in this case. Note that an @inline include should still include the synopsis in its inclusion.

I basically think that the comment attached to a module type is a description of the module type itself and the top comment on a signature is a description of any module that has that exact signature. Clearly a module whose type includes a module type is not equal to that module type -- so it shouldn't get the comment attached to the module type. A module whose type includes a signature is also not a module that has that exact signature -- it has a larger type which includes the contents of that type -- so it also shouldn't get the top comment of the signature. The second point is debatable, but I think that model makes sense.

@lpw25
Copy link
Contributor

lpw25 commented Mar 18, 2021

The second point is debatable, but I think that model makes sense.

It's so debatable that I think I've changed my mind. I think users would be less surprised by the behaviour in this patch than by the one that drops the whole synopsis. So I'm fine with what is implemented here.

@dbuenzli
Copy link
Contributor

I think that we should always drop the synopsis of the includee.

I'm fine with that, but I'm not fine with making a distinction between the declaration comment and the top comment.

Absolutely not. The top-comment is handled separately to the decl comment until the last stage where it is used to compose the preamble (and the synopsis)

Then it should not :-) If there is one what's the rationale for that behaviour ?

Now that we finally have clear definitions of synopses and preambles we should actually use them and that means not caring about how these get defined in the source code in terms of declaration and top docstring.

Again it's likely not the same person who includes than the one who wrote the thing to include. So it will make the behaviour seemingly random to includers for absolutely no good reason.

.

@Julow
Copy link
Collaborator Author

Julow commented Mar 19, 2021

If there is one what's the rationale for that behaviour ?

The next sentence, that you elided, is:

This is done on purpose, see #478 (comment)

Now that we finally have clear definitions of synopses and preambles

The preamble is defined in term of the top-comment and the declaration comment. Consider that the expansion occurs before the preamble can be defined, eg. module M : T defines M, not an alias to T.

@dbuenzli
Copy link
Contributor

This is done on purpose, see #478 (comment)

Well that comment precisely tells that a top-comment and declaration comment are the same.

But anyways… I'm getting a bit tired of having to argue to get a consistent design for end-users.

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 a pull request may close this issue.

4 participants