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

Manual: consistency test for cross-references to the manual #1556

Merged
merged 3 commits into from Jan 4, 2018

Conversation

Projects
None yet
2 participants
@Octachron
Copy link
Contributor

Octachron commented Jan 3, 2018

As a follow-up to #1554, this PR proposes to add a cross-referency consistency test to the manual:
This new test located in manual/test/cross_reference_checker.ml checks that all expressions and let bindings annotated [@manual.ref "label"] in a ml are still in-sync with the current version of the manual. As an illustration, the second commit in this PR adds the following annotation:

let ref_manual_explanation ppf =
  let[@manual.ref "s:comp-warnings"] chapter, section = 9, 5 in
  Format.fprintf ppf "(See manual section %d.%d)" chapter section

This test works by comparing the tuple literal in these expressions with the TeX label description in manual/manual/texstuff/manual.aux. An error is raised if there is a mismatch between references:

  let[@manual.ref "s:comp-warnings"] chapter, section = 9, 4

File "/…/utils/warnings.ml", line 309, characters 2-60:
Error: References for label "s:comp-warnings" do not match:
OCaml side 9.4,
manual 9.5

or if the label is unknown on the TeX side:

  let[@manual.ref "s:comp-warning"] chapter, section = 9, 4

File "/…/utils/warnings.ml", line 309, characters 2-59:
Error: Unknown manual label: s:comp-warning

(there are few more technical errors if the attribute payload is not a string literal or if the inner expression is not an integer tuple literals or an integer literal).

Since the test requires the manual.aux file, it can only be run after building the manual. However, this seemed a sensible constraint for a test that only needs to be run after an update to the manual itself.
Another option would be to parse the various tex and etex files to reconstruct this information by hand, with the issue that parsing TeX is either brittle or hideous.

Moreover, I moved the preexisting consistency test check-stdlib-modules from manual/manual/library to this new manual/tests folder to regoup executable files with similar concerns.

@gasche
Copy link
Member

gasche left a comment

Impressive work, thanks! The generality of the solution is very nice.

@@ -1,5 +1,5 @@
all: tools
cd manual; ${MAKE} all
cd manual; ${MAKE} all && cd ..; ${MAKE} tests

This comment has been minimized.

@gasche

gasche Jan 3, 2018

Member

doesn't it work to have it as a second line with just ${MAKE} tests?

This comment has been minimized.

@Octachron

Octachron Jan 3, 2018

Author Contributor

This works better indeed, fixed.

@@ -471,10 +477,10 @@ let message = function
| Expect_tailcall ->
Printf.sprintf "expected tailcall"
| Fragile_literal_pattern ->
Printf.sprintf
Format.asprintf

This comment has been minimized.

@gasche

gasche Jan 3, 2018

Member

I am skeptical that mixing Printf and Format is a good idea in general. it seems safe enough here as we use the sprintf variants anyway, but why not stick to Printf everywhere?

This comment has been minimized.

@Octachron

Octachron Jan 3, 2018

Author Contributor

Good point, fixed.

@gasche

gasche approved these changes Jan 3, 2018

Copy link
Member

gasche left a comment

This looks good to me. @Octachron, please do any rebasing that you think is appropriate and then merge.

@Octachron Octachron force-pushed the Octachron:manual_reference_check branch from c6d6ebe to a6978da Jan 4, 2018

@Octachron Octachron merged commit 77d7540 into ocaml:trunk Jan 4, 2018

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.

Copy link
Contributor Author

Octachron commented Jan 4, 2018

Minor fixes have been squashed, reviewer cited and PR merged.

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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.