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

Better handling of objects type definition #6274

Closed
vicuna opened this issue Dec 16, 2013 · 4 comments
Closed

Better handling of objects type definition #6274

vicuna opened this issue Dec 16, 2013 · 4 comments

Comments

@vicuna
Copy link

vicuna commented Dec 16, 2013

Original bug ID: 6274
Reporter: @trefis
Assigned to: @zoggy
Status: closed (set by @xavierleroy on 2015-12-11T18:27:40Z)
Resolution: open
Priority: normal
Severity: feature
Category: ocamldoc
Tags: patch
Monitored by: @gasche @hcarty

Bug description

Definition of object types, while syntactically similar to record types, are not "handled" by ocamldoc, i.e. the documentation one associates to fields does not appear in ocamldoc output.
It would be nice if it did.

I have written a few patches to implement such a feature, they are available for review at https://github.com/trefis/ocaml/tree/ocamldoc .

I'm not sure how much this is related to #4347 .
I probably have a naïve approach here, however it does seem to work without breaking anything and I am not yet "deeply sad to the point of not using ocamldoc at all" (to quote jm).

You will notice that I introduce some code duplication with the handling of records (although it might be hard to notice, considering how much code duplication there already is) which should be easily factorisable.
However I wanted to gauge people's interest before investing more time on that; and it can always be cleaned afterward.

Additional information

The patches are currently on the 4.01.0 branch, but I don't think rebasing them to trunk would pose any problem.

@vicuna
Copy link
Author

vicuna commented Dec 24, 2013

Comment author: @gasche

I just did a review of the patch (medium precision, I would say). It consists of three commits:

  1. trefis@759a72a collects comments occurring after method fields in a definition of an object type
  2. trefis@68fc982 collects method fields of object type definitions in the Odoc_type.t_type structure, and then pretty-prints them (in various backends) with more structure than just outputting the source text, rather like record definitions.
  3. trefis@501f15c fixes a bug with the implementation approach of commit (2)

I think (commit 1) is reasonable, but I am less convinced by the overall design of (commit 2): a new "type kind" is added to the type Odoc_type.type_kind, which previously precisely mirrored the type_kind of the typedtree (type synonym, variant or record), to store the information needed by the backends.

I think it is wrong to break the correspondence between the typedtree "type kind" and ocamldoc's "type kind" (for example ocamldoc uses type_kind to decide whether to print parameters variance information, and I suspect this is broken by this change). A better design, I think, would be to add the idea that, besides the type_kind, useful printing information may come from the type manifest (the type-expression after the equal sign, if any), and have a function like

val manifest_structure : Types.type_expr -> Odoc_type.ty_manifest option

where ty_manifest would be a new type storing information for the type expressions we decide to print in a special way when they occur on the right of an equal sign -- (only) objects types for now. At printing points you could check both the kind and the manifest, and decide on both (eg. giving priority to the ty_manifest when it exists).

I think most of the current implementation could be reused, the idea is only to preserve the tight correspondence between typedtree's and ocamldoc's type_kind.

Other than that the patch looks sensible, and I don't see any reason not to include this extra feature.

@vicuna
Copy link
Author

vicuna commented Dec 27, 2013

Comment author: @trefis

Thank you for your comments.

I tried to follow your proposition:
I first reverted the two previous commits, i.e. "2" and "3" in your comment, and then introduced [Odoc_type.type_manifest] as suggested (all this could be squashed before being pushed to the svn if you want a cleaner history).

I tried to make this commit as little intrusive as I could, so there is a lot of code redundancy which I then remove in the two next commits.

The code is available at the same url as before.

Cheers.

@vicuna
Copy link
Author

vicuna commented Jan 10, 2014

Comment author: @trefis

Quick update: gasche commented the previous patches directly on github.
I have since added one last patch to answer his comments.

Everything should be "OK" now.

PS: Thanks to "Ditoran2" for the random pokemon "maps". I hadn't realized mantis could be a suitable replacement to megaupload.

@vicuna
Copy link
Author

vicuna commented May 22, 2014

Comment author: @zoggy

I applied the patched in version/4.02 (rev 19405 and 19406).

@trefis: can you give it a try ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant