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

MPR7546, manual: preambles and warnings for compiler-libs modules #2020

Merged
merged 2 commits into from Sep 12, 2018

Conversation

Projects
None yet
5 participants
@Octachron
Copy link
Contributor

commented Sep 5, 2018

This PR ensures that all non-generated and non-internal compiler-libs modules available in the manual
are introduced by a short preamble. It also adds some words of caution on the instability of those module interfaces as suggested by @gasche in #2017 . Finally, it exposes some preexisting documentation comments, notably in Config .

@pmetzger
Copy link
Member

left a comment

This looks good! One minor comment below.

(** Lambda simplification and lambda plugin hooks *)
(** Lambda simplification and lambda plugin hooks
Plugins: beware this module makes no compatibility guarantees.

This comment has been minimized.

Copy link
@pmetzger

pmetzger Sep 5, 2018

Member

I'd say "beware, this module is not guaranteed compatible between releases." to make the meaning clearer. (The comma is also needed.)

(** Driver for the parser, external preprocessors and ast plugin hooks *)
(** Driver for the parser, external preprocessors and ast plugin hooks
Compiler-libs: beware this module makes no compatibility guarantees.

This comment has been minimized.

Copy link
@pmetzger

pmetzger Sep 5, 2018

Member

Same suggestion as above. I'll leave off the repetition of the same thing from here.

@gasche

This comment has been minimized.

Copy link
Member

commented Sep 5, 2018

I'm not fully satisfied with the wording you chose, but I'm even less satisfied by the fact that I can't find a way to avoid repeating the same warning in all files, besides manual HTML post-processing (which pleasantly doesn't scale to other ocamldoc output formats) or specialized generators (which we probably want to avoid). I thought -intro could be used for it.

I think we want a warning to convey more ideas:

  • This is part of the internal OCaml compiler API, not the language standard library.
  • There are no compatibility guarantees, so code written against these modules must be willing to depend on specific OCaml compiler versions.

Currently only the latter is said (partly).

@pmetzger

This comment has been minimized.

Copy link
Member

commented Sep 5, 2018

I think we want a warning to convey more ideas:

  • This is part of the internal OCaml compiler API, not the language standard library.
  • There are no compatibility guarantees, so code written against these modules must be willing to depend on specific OCaml compiler versions.

Currently only the latter is said (partly).

Why not say just that? Here's my suggestion. I've adjusted a few words:

This library is part of the internal OCaml compiler API, and is not the language standard library.
There are no compatibility guarantees between releases, so code written against these modules is not portable between OCaml compiler versions.

@dbuenzli

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2018

It should be understood that compiler-libs is unstable. If the module lists end up being split it should be enough to mention it before the list of compiler-libs modules.

@gasche

This comment has been minimized.

Copy link
Member

commented Sep 5, 2018

@dbuenzli: the attack mode I think is "beginner searches for some function on the web and ends up directly on a module page", so I would be more reassured by a warning on each page. Would your approach show this intro-text on each page?

@Octachron Octachron force-pushed the Octachron:manual_preamble_for_compilerlibs branch from 51ee294 to 6f91864 Sep 5, 2018

@dbuenzli

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2018

Would your approach show this intro-text on each page?

I'm not sure exactly what my approach is since I'm not sure what #2017 does. In any case it's better not to go the post-processing way since this won't appear in cmti-based docset generation (e.g. odig).

I think best would be to identify the main module M of compiler libs (is there one ?) and add a header (** {1:warning ...} and section explaining all that there. In each of the modules part of compiler libs you can then simply add e.g.:

(** {b Warning} This module is unstable and part of {{!M.warning}[compiler-libs]}. *)

If you can make the warning fit the header you could benefit from ocamloc's title splicing by simply doing:

(** {b Warning} {!M.warning} *)
@Octachron

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2018

@dbuenzli , #2017 merely builds the documentation for compiler-libs and stdlib separatedly (losing cross-references). I could add a custom index file, but there is already a short introduction .

@gasche , another option would be to preprocess the module and insert the warning, but manually adding the warning once seems fine to me? Eventually, I could add a script to update the warning.

@gasche

This comment has been minimized.

Copy link
Member

commented Sep 5, 2018

I'm ok with adding the warning once. I like @dbuenzli's suggestion to have a short explanation and factorize a longer discussion in a link, but I guess the PR with just a good wording in each modules is also fine by me.

@Octachron Octachron force-pushed the Octachron:manual_preamble_for_compilerlibs branch from 6f91864 to 227834c Sep 10, 2018

@Octachron

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2018

Inspired by @dbuenzli 's suggestion, I pushed a new implementation that adds an introduction file to compilerlibs, with the extended warning suggested by @pmetzger .

This introduction is then used as both an index and as a text-only module. Invidual modules (e. g. Ast_iterator) in compilerlibs contain now a short warning and a link to the Compilerlibs introduction. (The duplication of the introduction is due to my failure to link directly to the index).

@Octachron Octachron force-pushed the Octachron:manual_preamble_for_compilerlibs branch from 227834c to cdcbcd4 Sep 10, 2018

@gasche

gasche approved these changes Sep 10, 2018

Copy link
Member

left a comment

Very nice, thanks!

@dbuenzli

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2018

Just one comment I would prefer Compiler_libs rather than Compilerlibs. That may seem an obscure request but the library directory is called compiler-libs not compilerlibs. In general it's better for external tooling if we can have trivial mappings between module names/library names/directory names/opam package names/ocamfind names, you name it.

@Octachron

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2018

This is a good point. I went further and changed the name to Compiler-libs since text-only documentation pages are not limited to syntactically correct module names; and this drives home the point that the introduction page is not associated to a concrete module.

@Octachron Octachron force-pushed the Octachron:manual_preamble_for_compilerlibs branch from cdcbcd4 to c485466 Sep 11, 2018

@Octachron Octachron force-pushed the Octachron:manual_preamble_for_compilerlibs branch from c485466 to 5383e23 Sep 11, 2018

@Octachron Octachron merged commit ef135a7 into ocaml:trunk Sep 12, 2018

2 checks passed

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

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2018

This is a good point. I went further and changed the name to Compiler-libs since text-only documentation pages are not limited to syntactically correct module names;

Are you sure odoc won't choke on a {!Compiler-lib} reference. Saying this because I know for sure that heading identifiers (as in {1:id) can't have dashes. Maybe @trefis remembers.

@trefis

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2018

@aantron rewrote the parser so he'd be the one to know the answer here.

@trefis

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2018

Actually, I have the answer: the git version of odoc does choke on the - (see below).
And it actually make sense, we've reserved the use of - for namespace disambiguation, cf. https://ocaml-doc.github.io/octavius/syntax.html#references_syntax.

(cd _build/default/_doc/_odoc/lib/ocamlcommon@. && odoc compile -I . -I ../stdlib@. --pkg ocamlcommon@. -o terminfo.odoc ../../../../.ocamlcommon.objs/terminfo.cmti)
odoc: internal error, uncaught exception:
      Parser___Helpers.InvalidReference("unknown qualifier `Compiler'")
      Raised at file "src/parser/helpers.ml", line 28, characters 14-72
      Called from file "src/parser/helpers.ml", line 258, characters 28-47
      Called from file "src/parser/helpers.ml", line 338, characters 10-38
      Called from file "src/parser/syntax.ml", line 162, characters 28-52
      Called from file "src/parser/syntax.ml", line 345, characters 20-66
      Called from file "src/parser/syntax.ml", line 363, characters 16-34
      Called from file "src/parser/syntax.ml", line 370, characters 17-54
      Called from file "src/parser/syntax.ml", line 732, characters 18-33
      Called from file "src/parser/syntax.ml", line 1096, characters 6-71
      Called from file "src/model/error.ml", line 60, characters 9-15
      Called from file "src/parser/parser_.ml", line 88, characters 8-33
      Called from file "src/loader/attrs.ml", line 78, characters 4-155
      Called from file "src/loader/attrs.ml", line 98, characters 32-60
      Called from file "src/loader/cmti.ml", line 567, characters 16-43
      Called from file "src/loader/cmti.ml", line 590, characters 25-62
      Called from file "list.ml", line 117, characters 24-34
      Called from file "src/loader/cmti.ml", line 588, characters 4-135
      Called from file "src/loader/cmti.ml", line 597, characters 14-46
      Called from file "src/loader/loader.ml", line 43, characters 33-67
      Called from file "src/model/error.ml", line 60, characters 9-15
      Called from file "src/odoc/compile.ml", line 21, characters 8-36
      Called from file "src/cmdliner_term.ml", line 27, characters 19-24
      Called from file "src/cmdliner.ml", line 106, characters 32-39
@trefis

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2018

Thanks for your vigilance @dbuenzli.
@Octachron can we change the dash for an underscore?

@Octachron

This comment has been minimized.

Copy link
Contributor Author

commented Sep 12, 2018

Yes, being future-proof is clearly a good idea. I am going to propose a PR to change the name later today.

@dbuenzli

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2018

Yes, being future-proof is clearly a good idea

Note that as far as I'm concerned the future is already here. I always odoc my opam switches via odig and that includes compiler's libs interfaces, see for example here.

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.