Conversation
ocamldoc/odoc_messages.ml
Outdated
| let merge_raised_exception = ('e', "merge @raise") | ||
| let merge_return_value = ('r', "merge @return") | ||
| let merge_custom = ('c', "merge custom @-tags") | ||
| let merge_alert = ('x', "merge alerts") |
There was a problem hiding this comment.
Every letters of alerts are taken except t and I thought alerts are some kind of next-gen tags and needed a fancy letter.
| else | ||
| None | ||
| in | ||
| let comment_opt = Odoc_sig.analyze_alerts comment_opt val_desc.Parsetree.pval_attributes in |
There was a problem hiding this comment.
That's clever and probably the best way to source attributes information from the AST while keeping the separated extraction of documentation comments.
|
In term of features, that's look like a good idea (even more so if that avoid delay a little the divergence of tags/attributes support between ocamldoc and odoc). |
ocamldoc/odoc_sig.ml
Outdated
| in | ||
| let name = Name.parens_if_infix name_pre.txt in | ||
| let subst_typ = Odoc_env.subst_type env type_expr in | ||
| let comment_opt = analyze_alerts comment_opt value_desc.Parsetree.pval_attributes in |
There was a problem hiding this comment.
This is too early to merge alert and deprecation information: at this point, we have not yet collected the post-element documentation comments. Thus if we transfer the deprecation alerts to the deprecation info, when calling
merge_infos v.val_info info_after_optlater we end up merging the alert and the deprecation comment.
This is what is happening in the html test.
There was a problem hiding this comment.
Thanks for the debugging ! I think I've found a fix.
c081dda to
1fcf3f4
Compare
[@@deprecated] is treated the same as [@@Alert deprecated]. Alerts are rendered one per line, at the end of the comment.
Fits better with the way the 'info' record is passed around and rendered. Fix alerts not being rendered when there's not also a comment, with the previous approach.
Remove the deprecated alert if the tag is present. Lift the alert into the tag otherwise.
And render a '.' after the alert name. This is how the deprecated tag is rendered. We expect a sentence in the payload.
This function is no longer uptodate but can be removed instead of fixed.
194b13a to
7c263d2
Compare
|
Thanks for the review. Everything is fixed. |
|
Sorry, there is a small feature gap that I had missed earlier: we want to handle compilation unit-wide deprecation and alerts: (* a.mli *)
[@@@deprecated "Dont' use" ]Not handling deprecation on constructors or fields in perfectly fine for ocamldoc, but we probably want to handle that compilation unit case. |
Look for signature-item attributes ([@@@...]) at the beginning of a signature. These alerts are attached to the whole unit, similarly to the preamble.
Alerts attached to the module declaration and alerts at the beginning of
the signature are handled (even though a comment at this position
wouldn't be picked as the preamble):
module M : sig
[@@@deprecated "foo"]
end
module N : sig
end
[@@deprecated "foo"]
s module types
|
I just added support for unit-wide alerts and also for alerts attached to modules and module types (also "signature wide" alerts). |
|
Like I said, those cases are probably less important. But I will gladly review the code if you want to support them. |
Constructors, fields and all signature items.
|
Added a commit for this. |
ocamldoc/odoc_sig.ml
Outdated
| let comment_opt = analyze_alerts comment_opt pmd_attributes in | ||
| let comment_opt = | ||
| match module_type.Parsetree.pmty_desc with | ||
| | Parsetree.Pmty_signature sg -> analyze_toplevel_alerts comment_opt sg |
There was a problem hiding this comment.
Toplevel alerts are only recognized by the compiler at the compilation-unit level:
module M :sig [@@@deprecated] enddoes not attach the alert to M currently.
This is already slightly confusing, I would rather not have ocamldoc add more confusion here.
The compiler doesn't do that, so this is inconsistent.
ocamldoc/odoc_args.ml
Outdated
| (M.merge_raised_exception, [Odoc_types.Merge_raised_exception]) ; | ||
| (M.merge_return_value, [Odoc_types.Merge_return_value]) ; | ||
| (M.merge_custom, [Odoc_types.Merge_custom]) ; | ||
| (M.merge_alert, [Odoc_types.Merge_alert]) ; |
There was a problem hiding this comment.
Thinking a bit about this point, this is not the right semantic for alerts: alerts from the implementation are never merged nor lifted into the interface. This is not an user controlled behavior on the compiler side and thus it should not be an option on the ocamldoc side.
There was a problem hiding this comment.
Unrelated to this PR but isn't it weird that documentation comments aren't hidden by an interface file ? The generated documentation has more content than what's in the mli file and some documentation might be incomplete when switching from ocamldoc to odoc, which doesn't look at the implementation if it founds an interface.
There was a problem hiding this comment.
This is part of the behavior of ocamldoc that was intentionally not reproduced in odoc, because merging information from two slightly incompatible different sources can be really confusing (as illustrated by the @param tag).
There was a problem hiding this comment.
I tweaked the merge but a bug remain: The deprecated alert is converted to a deprecated tag too early and will be merged anyway.
This is was on purpose initially but I'm happy to change that if you think we should. It would be a big change though.
There was a problem hiding this comment.
No, that's enough time spent on ocamldoc. At this point, it is more a bug on the ocamldoc implementation of deprecation tags.
| [] -> | ||
| (acc_env, acc) | ||
| | {Parsetree.pvb_pat=pat; pvb_expr=exp} :: q -> | ||
| | {Parsetree.pvb_pat=pat; pvb_expr=exp;pvb_attributes=attrs} :: q -> |
There was a problem hiding this comment.
The behavior for single ml is now desynchronized from the behavior for interface file since we are only lifting alerts on value to the documentation side.
It is fine to not handle implementation only alerts, but it seems better to not partially handle them.
There was a problem hiding this comment.
Added the feature to the implem frontend too.
There was a problem hiding this comment.
Great! Thanks a lot for all the time spent on those details.
And be sure that alerts from an implementation are not rendered if an interface is present.
Octachron
left a comment
There was a problem hiding this comment.
Independently of #10983, surfacing all alert attributes in the generated documentation is a good change. In particular, this PR makes makes it much easier to keep the deprecation message synchronized between the alert and the documentation.
Follow-up to the discussion in #10983 about alerts and tags.
Tags and alerts are have intersecting jobs but only alerts are inspected by the compiler and eventually other tools. As @dbuenzli noted in #10983 (comment), it is annoying to have to write the
deprecatedannotation for the documentation and for the compiler separately.For example, the stdlib doesn't use the
@deprecatedtag.This PR makes ocamldoc recognize alerts attached to values, which are rendered similarly to the deprecated tag:
Alert foo. Body of the alert.
The deprecated alert is lifted into the deprecated tag for better rendering and to avoid rendering it twice.
[@@deprecated]and[@@alert deprecated]are treated as synonyms.This is how the documentation of the stdlib changes:
Details