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

Ocamldoc: warning for missed cross-reference opportunities #1017

Merged
merged 3 commits into from Feb 5, 2017

Conversation

Projects
None yet
3 participants
@Octachron
Contributor

Octachron commented Jan 21, 2017

Following a discussion with @dra27 in #1016, this PR proposes to add an option to ocamldoc to identify missed cross-reference opportunities.

More precisely, with the -warn-missed-cross option enabled, code fragments of the form [path] that correspond to a known element and could therefore be replaced by {!path} trigger a warning.

To decrease the false positive rate, the current patch does not trigger the warning if:

  • the identifier is the same as the parent of the current element, e.g
val f: unit -> unit
(** [f] does not trigger the missed cross-reference warning *) 
  • the path of the identifier is a suffix of the path of the parent element,
val f: unit -> unit
(** [Grand_parent.Parent.f] also does not trigger the missed cross-reference warning *) 

The second commit in this PR replaces few occurences of this warning by cross-references.
Note that even with the implemented heuristic, the warning is unfortunately too noisy to be activated by default.

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Jan 21, 2017

Contributor

Nice! Don't know if anyone else wants to comment on the command line flag - it all looks good to me.

Contributor

dra27 commented Jan 21, 2017

Nice! Don't know if anyone else wants to comment on the command line flag - it all looks good to me.

Show outdated Hide outdated ocamldoc/odoc_messages.ml
@@ -323,6 +324,12 @@ let cross_type_not_found n = "Type "^n^" not found"
let cross_recfield_not_found n = Printf.sprintf "Record field %s not found" n
let cross_const_not_found n = Printf.sprintf "Constructor %s not found" n
let code_could_be_cross_reference n parent =
Printf.sprintf "Code element [%s] in %s corresponds to a known \
cross-referenceable element, it might be worthwile to replace it \

This comment has been minimized.

@dra27

dra27 Jan 21, 2017

Contributor

worthwhile (second h missing)

@dra27

dra27 Jan 21, 2017

Contributor

worthwhile (second h missing)

This comment has been minimized.

@Octachron

Octachron Jan 22, 2017

Contributor

Fixed, thanks!

@Octachron

Octachron Jan 22, 2017

Contributor

Fixed, thanks!

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Jan 21, 2017

Contributor

One other, given that there are lots of false positives and there's no way to suppress the warnings, would -show-missed-cross (or suggest?) be a good alternative?

Contributor

dra27 commented Jan 21, 2017

One other, given that there are lots of false positives and there's no way to suppress the warnings, would -show-missed-cross (or suggest?) be a good alternative?

@Octachron

This comment has been minimized.

Show comment
Hide comment
@Octachron

Octachron Jan 22, 2017

Contributor

I like the -show prefix. For clarity sake, I went for -show-missed-crossref, since three more letter seemed a low price to paid for a quite specific option.

Concerning false positives, I took the time to count false positive in the standard library and compiler library (excluding the bigarray library):

Exceptions Module/constructor confusion Module confusion Total
8 5 15 28

These false positives can be classified in three categories:

  • exception:
exception E
(** In this documentation comment, ocamldoc does not give accurate parent information, 
      consequently [E] is detected has a missed crossreference opportunity
*) 
  • Module/constructor confusion: constructor and module sharing the same name, like Arg.String and
    String.

  • Module confusion: specific to the labeled version of the standard library in which modules are confused about their own identity (i.e. ArrayLabels refers to Array in documentation commenst).

Contributor

Octachron commented Jan 22, 2017

I like the -show prefix. For clarity sake, I went for -show-missed-crossref, since three more letter seemed a low price to paid for a quite specific option.

Concerning false positives, I took the time to count false positive in the standard library and compiler library (excluding the bigarray library):

Exceptions Module/constructor confusion Module confusion Total
8 5 15 28

These false positives can be classified in three categories:

  • exception:
exception E
(** In this documentation comment, ocamldoc does not give accurate parent information, 
      consequently [E] is detected has a missed crossreference opportunity
*) 
  • Module/constructor confusion: constructor and module sharing the same name, like Arg.String and
    String.

  • Module confusion: specific to the labeled version of the standard library in which modules are confused about their own identity (i.e. ArrayLabels refers to Array in documentation commenst).

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Jan 23, 2017

Contributor

The exception one is unfortunate, although it sounds as though all three of those may be the case that they affect the standard library in ways which might be less usual in user-code.

@gasche - any thoughts?

Contributor

dra27 commented Jan 23, 2017

The exception one is unfortunate, although it sounds as though all three of those may be the case that they affect the standard library in ways which might be less usual in user-code.

@gasche - any thoughts?

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Jan 23, 2017

Member

I don't have the time to think about this unfortunately. I would merge the .mli polish with my eyes closed, the rest of the PR needs someone (possibly one of you two) to make a decision.

Member

gasche commented Jan 23, 2017

I don't have the time to think about this unfortunately. I would merge the .mli polish with my eyes closed, the rest of the PR needs someone (possibly one of you two) to make a decision.

@dra27

This comment has been minimized.

Show comment
Hide comment
@dra27

dra27 Jan 23, 2017

Contributor

In which case, in the absence of anyone expressing concern, I move to merge this.

Contributor

dra27 commented Jan 23, 2017

In which case, in the absence of anyone expressing concern, I move to merge this.

@Octachron

This comment has been minimized.

Show comment
Hide comment
@Octachron

Octachron Feb 2, 2017

Contributor

Since the change in ocamldoc is quite light, I don't see any drawback to add a middly useful option to ocamldoc. Consequently, if there are no objections, I will merge by the end of the week.

Contributor

Octachron commented Feb 2, 2017

Since the change in ocamldoc is quite light, I don't see any drawback to add a middly useful option to ocamldoc. Consequently, if there are no objections, I will merge by the end of the week.

@Octachron Octachron merged commit cdb0208 into ocaml:trunk Feb 5, 2017

2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
@Octachron

Octachron Feb 5, 2017

Contributor

Merged.

Contributor

Octachron commented Feb 5, 2017

Merged.

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

Merge pull request #1017 from Octachron/potential_cross_reference_war…
…ning

Ocamldoc: option to show missed cross-reference opportunities
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment