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: signature option for caml_example #1702

Merged
merged 6 commits into from May 5, 2018

Conversation

Projects
None yet
2 participants
@Octachron
Copy link
Contributor

Octachron commented Apr 6, 2018

This small PR extends the options of the caml_example pseudo-environment with a signature mode. With this PR, the possible modes for caml_example are:

  • toplevel, for examples that should look as if they were send to the toplevel
  • verbatim, for generic code examples
  • signature, for examples of interface files or signature

This seems to cover most documentation needs in the manual, and it is sufficient to convert the faulty example fixed by @lpw25 in #1693 .
In term of implementation, the code inside the environment is merely wraped inside module type Wrap = sig ... end before being send to the ocaml subprocess.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Apr 6, 2018

I am not sure whether the sig .. end wrapping mechanism works correctly (or whether it is going behave in weird/surprising ways in some cases; what if we use the mode that also shows the toplevel output?). I assume you have tested it by converting one of the signatures in the manual, and checked that the output is as you expect? If so, maybe this change could be part of the patch as well?

@Octachron

This comment has been minimized.

Copy link
Contributor Author

Octachron commented Apr 6, 2018

For more concrete example, I finished the conversion of the example in ocamldoc: html and pdf . (The corresponding commit also fixes transf.mll to make sure that \camlexample is considered a verbatim mode).

what if we use the mode that also shows the toplevel output?

In this case, it would show the wrapping module type, I can see two options to avoid this case: either require to combine the signature mode with caml_example * or remove the wrapping module type. I tend for the first option.

@gasche

This comment has been minimized.

Copy link
Member

gasche commented Apr 6, 2018

Yes, the first option is simplest, and inferred signatures are not very useful anyway (they mostly repeat the signature).

Would you consider adding a Changes entry, though? This is a nice change that did require some work from you, and I like the idea of crediting changes to the manual and broader documentation ecosystem.

@gasche

gasche approved these changes Apr 6, 2018

Copy link
Member

gasche left a comment

Approved -- although I understand that you might want to make further tweaks. Feel free to merge whenever you are ready and the CI passes.

(This change caught other mistakes in the ocamldoc manual than the ones caught by Leo, which means that it was indeed a reasonable idea.)

&& mv $*.transf_error.tex $*.gen.tex\
&& $(TEXQUOTE) < $*.gen.tex > $*.texquote_error.tex\
&& mv $*.texquote_error.tex $*.tex\
|| printf "Failure when generating %s\n" $*.tex

This comment has been minimized.

@gasche

gasche Apr 6, 2018

Member

We are accumulating logic to use CAMLLATEX/TRANSF/TEXQUOTE in several different Makefiles (apparently cmds/Makefile does not use CAMLLATEX but only the other two?). Maybe it would make sense to factorize this in a shared makefile, or as a separate shellscript to invoke?

This comment has been minimized.

@Octachron

Octachron Apr 7, 2018

Author Contributor

This is probably a good idea for a next PR.

@Octachron Octachron force-pushed the Octachron:manual_signature_example branch from e1168ed to cc8bb43 Apr 7, 2018

@Octachron

This comment has been minimized.

Copy link
Contributor Author

Octachron commented Apr 7, 2018

I added an error message for the combination of \begin{caml_example}{signature}, and a change entry. I will merge after the merge of Leo's doctring fix.

@Octachron Octachron force-pushed the Octachron:manual_signature_example branch from cc8bb43 to b0cb561 May 5, 2018

@gasche

This comment has been minimized.

Copy link
Member

gasche commented May 5, 2018

I think we could keep this in 4.07, as it could be useful for further improve-the-manual changes (eg. a more complete version of #1209) that it could be worthwhile to include in 4.07 documentation. The present PR does not touch anything that is critical for the release preparation/testing, and it is customary to give external tools (ocamldoc, the manual build tools) more slack in freeze decisions.

@Octachron Octachron force-pushed the Octachron:manual_signature_example branch from b0cb561 to 5918e4e May 5, 2018

@Octachron

This comment has been minimized.

Copy link
Contributor Author

Octachron commented May 5, 2018

It is true that this is mostly a validation tool. I moved back the change entry to 4.07 .

@@ -12,9 +12,14 @@ TRANSF=$(SET_LD_PATH) $(OCAMLRUN) ../../tools/transf
TEXQUOTE=../../tools/texquote2
FORMAT=../../tools/format-intf

WITH_TRANSF= ocamldoc.tex top.tex intf-c.tex flambda.tex spacetime.tex \
CAMLLATEX= $(OCAMLRUN) ../../tools/caml-tex2 -caml "TERM=norepeat $(OCAML)" \
-n 80 -v false

This comment has been minimized.

@gasche

gasche May 5, 2018

Member

Does this not need an update to take 482e8a8 into account?

This comment has been minimized.

@Octachron

Octachron May 5, 2018

Author Contributor

You are totally right, I had forgotten this point. I should try to make my test environment sensitive to this problem. Fixed.

@gasche

gasche approved these changes May 5, 2018

Copy link
Member

gasche left a comment

Good to merge when CI passes.

@Octachron

This comment has been minimized.

Copy link
Contributor Author

Octachron commented May 5, 2018

I removed a wandering unused macros, since keeeping the macro count as low as possible seemed important enough to warrant a last minute fix. I will squash and merge once the CI passes.

@Octachron Octachron merged commit 145bedc into ocaml:trunk May 5, 2018

2 checks passed

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

Octachron added a commit that referenced this pull request May 5, 2018

manual: signature option for caml_example (#1702)
* Signature option for caml_example
* convert ocamldoc code example to caml_example
* error message when using caml_example*{signature} without *
@Octachron

This comment has been minimized.

Copy link
Contributor Author

Octachron commented May 5, 2018

Cherry-picked to 4.07 as d040ad6

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.