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

use the `caml_example` environment in the Extensions manual #1209

Merged
merged 3 commits into from Aug 27, 2018

Conversation

Projects
None yet
4 participants
@gasche
Copy link
Member

commented Jun 21, 2017

This PR, which continues work started in #939 as tracked in MPR#7551, has one well-defined commit that I believe is correct, but I marked it work-in-progress as:

  • the whole section of the manual is not covered
  • the parts that I did uncovered some minor details with the {caml_example}{verbatim} environment (from #1194) that we may want to discuss and potentially fix before going further

To that end I uploaded a temporary rendering of the PR manual.

If you look at the Recursive modules section for example, you may notice that

  1. The # at the beginning of the code example does not look nice (we are supposed not to be in the toplevel)
  2. The spacing at the end of the code is too big. See also the examples in private row types.
  3. It would be sort of nice to have a way to show signature item examples that are syntax- and type-checked, but there is no urgency.

I think that it would be useful to think (cc @Octachron !) about fixing (1) and (2) before considering merging this or going further with the caml_example-ization of this section.

@Octachron

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2017

For the first point, on the html side the # is implemented with a css rule. I would say the best way to remove it would be to add a verbatim (resp. toplevel) class to the caml-example div. This would allow to customize the style of both variant independently. On the latex side, removing the line of # on the first column might require some macro trickery. Note that the rendering of the output might benefit from some tuning too.

On the second point, the generated html code looks fine, this is probably a matter of tuning the margin/padding in the manual css style.

For 3, I think that wrapping the example in a module type definition before sending it to the toplevel might do the trick.

let a = A
let b n = assert (n > 0); B n
end
\end{caml_example}

This comment has been minimized.

Copy link
@Octachron

Octachron Jun 21, 2017

Contributor

May I suggest to add a star here \end{caml_example*} to not betray the illusion that caml_example is a latex environment? (pure nitpicking, I wholeheartedly admit)

\end{verbatim}
\begin{caml_example*}{verbatim}
type t = [ `A of int | `B of bool ]
\end{caml_example}

This comment has been minimized.

Copy link
@Octachron

Octachron Jun 21, 2017

Contributor

Another lost star above.

This comment has been minimized.

Copy link
@gasche

gasche Jun 21, 2017

Author Member

(Thanks! Fixed both.)

@gasche gasche force-pushed the gasche:manual-examples branch from 5a50b67 to b87edfd Jun 21, 2017

@gasche

This comment has been minimized.

Copy link
Member Author

commented Jun 23, 2017

I dislike LaTeX programming, so I don't plan to work on it. On the other hand I'll try to propose a patch for the html/css part along the lines proposed. (I'm interested in feedback on what future we want for this PR. Should we wait for the display issues to be fixed before considering a merge? Should we complete the porting of the chapter? (I would at least support the second option.))

@Octachron

This comment has been minimized.

Copy link
Contributor

commented Jun 23, 2017

Completing the porting of the current section seems like a good idea: it increases the number of examples available for tuning the rendering afterwards. Moreover, since the fix for the rendering should be mostly independent from the manual changes, I don't see a lot of negatives with merging this PR before the rendering the fix. ((In term of negatives, I imagine that the rendering changes might require some micro-tuning of the manual examples)).

@xavierleroy

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2017

Three months later, where are we on this GPR?

@gasche

This comment has been minimized.

Copy link
Member Author

commented Sep 14, 2017

I haven't worked on it since, and I would rather not merge the change before the rendering gets improved. On the other hand, it's a low-conflict PR so it can stay open for longer. Thanks for the WIP tag, it's appropriate.

@damiendoligez damiendoligez added this to the 4.07-or-later milestone Sep 25, 2017

@Octachron

This comment has been minimized.

Copy link
Contributor

commented May 4, 2018

@gasche , is there any obstacle (except the current conflict) remaining before merging?

@gasche

This comment has been minimized.

Copy link
Member Author

commented May 4, 2018

I don't remember. I will fix the conflicts and rebase, and then maybe you could think about it, and if applicable accept and merge?

@gasche

This comment has been minimized.

Copy link
Member Author

commented May 5, 2018

I'm looking at it now and it turns out that #1702 would be useful to go further on this PR.

@Octachron

This comment has been minimized.

Copy link
Contributor

commented May 5, 2018

I have rebased #1702 (and moved the change entry to trunk).

@gasche gasche force-pushed the gasche:manual-examples branch from b87edfd to 46a5f75 May 5, 2018

@gasche gasche changed the title [wip] use the new `caml_example` environment in the Extensions manual use the `caml_example` environment in the Extensions manual May 5, 2018

@gasche

This comment has been minimized.

Copy link
Member Author

commented May 5, 2018

I have rebased this PR, and added quite a few uses of caml_example. The remaining verbatim environments in exten.etex are for the cases where I don't know how to use caml_example instead, typically code that uses ... for ellipsis -- and thus wouldn't compile.

@gasche gasche force-pushed the gasche:manual-examples branch from 46a5f75 to 132ef5b May 5, 2018

@gasche

This comment has been minimized.

Copy link
Member Author

commented May 6, 2018

Re dots.: a lot of the manual examples look like

let f (type a) (x : a -> a) = ...

@Octachron, as the manual/tools wizard, do you know if it would be possible to render this out of something like

let f (type a) (x : a -> a) = (assert false)[@ellipsis]

?

@Octachron

This comment has been minimized.

Copy link
Contributor

commented May 6, 2018

It sounds possible. However, in order to have a robust behavior with attributes, the best choice is probably to parse each phrase and extract the location of each ellipsis from the parse tree.

@gasche

This comment has been minimized.

Copy link
Member Author

commented May 6, 2018

This is what is done for expect-tests in testsuite/tools/expect_test.ml.

While we are at it, another random feature request inspired by this PR: if I pass [error] or [warning=...], could I get the error or warning shown even in the {caml_example*} case? On my machine what I observe in the warning case is that the warning position is highlighted (a very nice feature in general), but the warning message is not shown.

Edit (August 2018): the question above has been fixed by #1863.

@gasche gasche force-pushed the gasche:manual-examples branch from 132ef5b to 6195a92 May 7, 2018

@gasche

This comment has been minimized.

Copy link
Member Author

commented May 7, 2018

I just rebased this against #1765.

let _ = Hashtbl.add devices "SVG" (module SVG : DEVICE)

module PDF = struct let draw () = ()[@@ellipsis] end
let _ = Hashtbl.add devices "PDF" (module PDF: DEVICE)

This comment has been minimized.

Copy link
@gasche

gasche May 7, 2018

Author Member

@Octachron I think that you may find this block of code interesting.

The first ellipsis is used in place of a separate {caml_eval} block as I had in my previous version (before the last rebase), and it suggests that maybe longer-term we could think of using [@hide] attributes that are completely removed instead of a separate environment.

The second ellipsis has this form, instead of a signature-item attribute, because of the issue I reported in https://github.com/ocaml/ocaml/pull/1765/files#r186431706. I am trying to come with a fix for the issue, but maybe we could consider trying to get this PR merged before that -- it's been long enough in the baking.

This comment has been minimized.

Copy link
@Octachron

Octachron May 7, 2018

Contributor

I agree that it would be nice to merge soon. I will do a full review by tomorrow.

that maybe longer-term we could think of using [@hide] attributes that are completely removed instead of a separate environment.

This is definitively doable. At the same time, I fear that mixing hidden and non-hidden code fragment too freely might be confusing for the reader.

This comment has been minimized.

Copy link
@gasche

gasche May 7, 2018

Author Member

Thinking more about it, I think we should wait until we agree on what to do with #1765 before merging.

@gasche gasche force-pushed the gasche:manual-examples branch from 6195a92 to 9cb4c0a May 7, 2018

@damiendoligez damiendoligez removed this from the consider-for-4.07 milestone May 29, 2018

@gasche gasche force-pushed the gasche:manual-examples branch from 9cb4c0a to 64a0cf4 Aug 19, 2018

@gasche

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2018

I just rebased this PR on top of #1903, and the ellipsis machinery now works correctly in all examples where it is used.

@Octachron note that I added a patch, currently 48ea928, to simplify the location-handling in the ellipsis code, now that attributes carry their whole location.

@Octachron
Copy link
Contributor

left a comment

I have three small remarks but overall the examples look nice.


let v = Point {width = 10; x = 0.; y = 0.}

let scale l = function
| Point p -> Point {p with x = l *. p.x; y = l *. p.y}
| ....
| Other -> Other

This comment has been minimized.

Copy link
@Octachron

Octachron Aug 26, 2018

Contributor

Was this change by choice or due to the lack of attribute on case? If it is the second, using
| Other[@ellipsis.start] -> Other[@ellipsis stop] would work after a small fix that I can send as a sub-PR.

This comment has been minimized.

Copy link
@gasche

gasche Aug 27, 2018

Author Member

In that case I think that showing the full code is just as well. In your sub-PR, can you include a test somewhere? I should also add a regression test for val picture : picture -> unit[@@ellipsis] which I had a lot of trouble not to get rendered as val picture : picture -> ....

This comment has been minimized.

Copy link
@Octachron

Octachron Aug 27, 2018

Contributor

I think I will make a test PR for ellipsis attributes in general.

module type DEVICE = sig [@@@ellipsis] end
let devices : (string, (module DEVICE)) Hashtbl.t = Hashtbl.create 17
type picture = unit[@ellipsis]
module type DEVICE = sig val draw : picture -> unit [@@ellipsis] end

This comment has been minimized.

Copy link
@Octachron

Octachron Aug 26, 2018

Contributor

I would propose to have a separate ellipsis [@@@ellipsis] to have

module type DEVICe = sig
val draw:picture -> unit
...
end

rather than just sig ... end since it gives a clearer purpose to picture.

@@ -409,7 +409,7 @@ CAMLTEX= ../compilerlibs/ocamlcommon.cma \
caml-tex: INCLUDES+= -I ../otherlibs/str -I ../otherlibs/$(UNIXLIB)
caml-tex: $(CAMLTEX)
../runtime/ocamlrun ../ocamlc -nostdlib -I ../stdlib $(LINKFLAGS) \
-linkall -o $@ $(CAMLTEX)
-linkall -o $@ -no-alias-deps $(CAMLTEX)

This comment has been minimized.

Copy link
@Octachron

Octachron Aug 26, 2018

Contributor

I am curious, what is the story behind this -no-alias-deps?

This comment has been minimized.

Copy link
@gasche

gasche Aug 27, 2018

Author Member

If you remove it, the build fails because Parsetree (to which an alias is introduced in #1903) has no .cmo to link against. This sounds strange, but module M = Foo is broken on all OCaml versions if Foo is mli-only and you don't use no-alias-deps.

@gasche gasche force-pushed the gasche:manual-examples branch from 64a0cf4 to 0c5e51f Aug 27, 2018

@gasche

This comment has been minimized.

Copy link
Member Author

commented Aug 27, 2018

I rebased on top of trunk, implementing @Octachron's recommendation (to show val draw in the DEVICE signature).

@gasche gasche force-pushed the gasche:manual-examples branch from 0c5e51f to acc2c70 Aug 27, 2018

gasche added some commits Aug 19, 2018

[minor] manual: simplify the refman/ build system
I wanted to test a caml_tex issue by creating a smaller .etex file
than exten.etex, and got bitten by the fact that the Makefile hardcode
the knowledge that only exten.etex uses caml_example.

@gasche gasche force-pushed the gasche:manual-examples branch from acc2c70 to 3e588dc Aug 27, 2018

@gasche gasche merged commit e687609 into ocaml:trunk Aug 27, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
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.