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

Small changes required for the upcoming "precompiled" version of learn-ocaml #35

Merged
merged 3 commits into from
Sep 8, 2023

Conversation

AltGr
Copy link
Collaborator

@AltGr AltGr commented Apr 22, 2022

Some quirks needed in porting to the pre-compiling version of learn-ocaml:

  • remove #install_printer (and any other) directives: we are no longer in a toplevel so these are no longer available. However, using tricky hacks, you can still define a printer for type foo with just let print_<foo> ppf x = ... Format.fprintf ppf ...

  • replacing open intended for shadowing with include in Prepare/Prelude
    (fpottier/unionfind/prepare.ml, mooc/week4/seq3/ex1/prepare.ml)
    Open can't reach outside of its compilation unit

  • renamed multiple definitions of exceptions/types/modules with the same name
    (fpottier/tictactoe/test.ml)
    this is allowed in the toplevel but not by the OCaml compiler

  • renamed toplevel functions named sample_* that don't type as samplers
    (mooc/week3/seq4/ex1/test.ml, mooc/week5/seq1/ex1/test.ml)
    These are dynamically typed later on, which just issues a warning, so we could
    disable this less precise check altogether for better compat, but it seems
    cleaner to reject them

  • avoid using a default sampler then overriding it
    (mooc/week5/seq2/ex1/test.ml)
    Due to difficulties in name resolution, this is forbidden, but the workaround
    is simple: just alias the built-in sampler locally before the first use.

Some quirks needed in porting to the pre-compiling version of learn-ocaml:

- replacing `open` intended for shadowing with `include` in Prepare/Prelude
  (fpottier/unionfind/prepare.ml, mooc/week4/seq3/ex1/prepare.ml)
  Open can't reach outside of its compilation unit

- renamed multiple definitions of exceptions/types/modules with the same name
  (fpottier/tictactoe/test.ml)
  this is allowed in the toplevel but not by the OCaml compiler

- renamed toplevel functions named `sample_*` that don't type as samplers
  (mooc/week3/seq4/ex1/test.ml, mooc/week5/seq1/ex1/test.ml)
  These are dynamically typed later on, which just issues a warning, so we could
  disable this less precise check altogether for better compat, but it seems
  cleaner to reject them

- avoid using a default sampler then overriding it
  (mooc/week5/seq2/ex1/test.ml)
  Due to difficulties in name resolution, this is forbidden, but the workaround
  is simple: just alias the built-in sampler locally before the first use.
Copy link
Member

@erikmd erikmd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @AltGr!

Question: regarding your comment

remove #install_printer (and any other) directives: we are no longer in a toplevel so these are no longer available. However, using tricky hacks, you can still define a printer for type foo with just let print_<foo> ppf x = ... Format.fprintf ppf ...

could you be more specific? I mean, from you PoV is there something to do in this PR (with the hack you suggest or so) before it is merged?

Otherwise, I propose to merge it as is now, as it is backward-compatible (cf. CI ✔️)

@AltGr
Copy link
Collaborator Author

AltGr commented Apr 28, 2022

could you be more specific? I mean, from you PoV is there something to do in this PR (with the hack you suggest or so) before it is merged?

EDIT: to be clear, the tricky hacks are hidden in the implementation of learn-ocaml, they're not in the exercises!

Normally it should be fine, what I meant is that registration of the printers now only relies on proper naming print_foo with the defined type foo in scope, and proper typing as Format.formatter -> foo -> unit (with extra arguments in front for the printing of the type parameters in the case of a parametric type, e.g. 'a foo, ('a, 'b) foo, etc.)

From my tests, the existing exercises don't leverage parametric type printing (it's a feature of OCaml with #install_printer already, but undocumented 🤷 ) — although the markov exercise typically would benefit from it ; and already have aptly named printers so they should work as expected with this PR.

@AltGr AltGr merged commit c3ba1b4 into ocaml-sf:master Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants