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

MPR#7635: ocamldoc, add missing ids to signature items #1383

Merged
merged 1 commit into from Sep 30, 2017

Conversation

Projects
None yet
4 participants
@Octachron
Copy link
Contributor

commented Sep 30, 2017

Mantis#7635:

Currently, ocamldoc html generator adds an unique id to most signature items: values, types, exceptions but also class types or extension constructors.

There is three notable missing elements in the above list: modules, module types and classes, which make it harder to link to these items.

This PR proposes to fix this shortcoming.

Note that for class items, the existing name attribute is replaced with an id attribute since a comment seems to indicate that this was the original intent.

@yawaramin
Copy link
Contributor

left a comment

LGTM!

@@ -36,6 +36,12 @@ let charset = ref "iso-8859-1"
(** The functions used for naming files and html marks.*)
module Naming =
struct
(** The prefix for modules marks. *)
let mark_module = "MODULE_"

This comment has been minimized.

Copy link
@yawaramin

yawaramin Sep 30, 2017

Contributor

The other marks don't seem to include the _ character, should we follow the same style for the new ones?

This comment has been minimized.

Copy link
@Octachron

Octachron Sep 30, 2017

Author Contributor

Yes, complicating the logic here was a bad idea. Fixed, thanks.

(**return the link target for the given module. *)
let module_target m = target mark_module (Name.simple m.m_name)

(**return the link target for the given module. *)

This comment has been minimized.

Copy link
@yawaramin

yawaramin Sep 30, 2017

Contributor

module type?

This comment has been minimized.

Copy link
@Octachron

Octachron Sep 30, 2017

Author Contributor

Well spotted, fixed.

@Octachron Octachron force-pushed the Octachron:ocamldoc_module_id branch 2 times, most recently from d143013 to e60e32b Sep 30, 2017

@gasche

gasche approved these changes Sep 30, 2017

Copy link
Member

left a comment

Approved modulo minor comment.

I agree with the choice to have only in trunk (not to backport in 4.06), that would be my natural choice as well.

Changes Outdated
@@ -22,6 +22,9 @@ be mentioned in the 4.06 section below instead of here.)

### Tools:

- MPR#7635: add an identifier to module and module type elements

This comment has been minimized.

Copy link
@gasche

gasche Sep 30, 2017

Member

should mention "ocamldoc"

This comment has been minimized.

Copy link
@Octachron

Octachron Sep 30, 2017

Author Contributor

Fixed, and added the missing reviewers field too.

@Octachron Octachron force-pushed the Octachron:ocamldoc_module_id branch from e60e32b to 57d5890 Sep 30, 2017

ocamldoc html: add missing id to signature items
This commit adds a new id to classes, modules and module types.
The class id replaces the preexisting name attribute that was intended
to be an id attribute.

@Octachron Octachron force-pushed the Octachron:ocamldoc_module_id branch from 57d5890 to 9593af5 Sep 30, 2017

@gasche gasche merged commit 5b1cffc into ocaml:trunk Sep 30, 2017

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gasche

This comment has been minimized.

Copy link
Member

commented Sep 30, 2017

Merged, thanks! And thanks @yawaramin for the bug report / feature request.

@hannesm

This comment has been minimized.

Copy link
Member

commented Oct 4, 2017

given other ocamldoc related changes in 4.06, could this please as well be considered to be merged into 4.06?

@gasche

This comment has been minimized.

Copy link
Member

commented Oct 4, 2017

Ok, I backported the change in 4.06: 42ef135

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.