Skip to content

Conversation

@erikmd
Copy link
Collaborator

@erikmd erikmd commented Jul 24, 2019

Kind: feature
Closes #50

Summary

I finally found the time to address what was proposed in #50, namely: implementing
an extension point [%prot: …] that allows one to generate a (_, _, _) prot at ppx time
an extension point [%funty: …] that allows one to generate a (_, _, _) fun_ty at ppx time.

This appears to be a key component for the learn-ocaml-editor automatic test.ml generator,
which will be able to automatically generate snippets such as

let q_divide =
  test_function_against_solution
  [%funty: int -> int -> int] "divide"
  []

Thus, no need to guess by regexp or so what is the number of arguments of the specified function
(unlike the more traditional part of the API test_function_N_against_solution with N among 1,2,3,4)

As mentioned in the commit messages, this PR includes a small refactoring (to move the prot-specific definitions in its own library) and two small tests incorporated in the demo.

Cc @yurug, @leunam217

@erikmd erikmd mentioned this pull request Jul 26, 2019
10 tasks
@yurug
Copy link
Collaborator

yurug commented Aug 11, 2019

Thanks Erik. That is a pretty neat PR and a great enhancement of our interface. I have a few comments though.

I have some doubts about the name Prot because I prefer short words over abbreviated long words and also because I am not sure that "prototype" is the best word here. Indeed, as said in the Wikipedia page, a prototype normally includes the name of a function, not just its type. Would "function type" be more accurate? You could shorten it as fun_ty or FunTy if you want.

Did you check that this does not break existing graders? For instance, you could check that by compiling the learn-ocaml-corpus repository.

@erikmd
Copy link
Collaborator Author

erikmd commented Aug 11, 2019

Dear @yurug, thanks for your feedback!

I have some doubts about the name Prot because I prefer short words over abbreviated long words and also because I am not sure that "prototype" is the best word here. Indeed, as said in the Wikipedia page, a prototype normally includes the name of a function, not just its type. Would "function type" be more accurate? You could shorten it as fun_ty or FunTy if you want.

Indeed, I agree that a "function type" would be a more precise naming than a "prototype". So I'm going to replace all occurrences of prot with fun_ty, and [%prot: …] with [%funty: …].

Did you check that this does not break existing graders? For instance, you could check that by compiling the learn-ocaml-corpus repository.

Good point; I've tested the current version of the PR (as-is, and after merging it locally with latest master) for compiling the learn-ocaml-corpus repository successfully − modulo the bug that I reported in #305, which appears to be an independent bug reproducible on learn-ocaml/master.

@erikmd
Copy link
Collaborator Author

erikmd commented Aug 12, 2019

Dear @yurug, I updated the PR as you suggested (using fun_ty instead of prot, and documenting the new interface fun_ty.mli).

Regarding backward-compatibility, as I mentioned previously learn-ocaml-corpus is not impacted by the PR, but some of our PFITAXEL graders did not pass at first (either due to a missing open Fun_ty or as ty_of_prot was renamed to ty_of_fun_ty). This led me to refactor my PR further (see the last commit refactor: Ensure …, which involves a deprecated synonym for ty_of_fun_ty) so these existing graders pass with no change.
Thus, no breaking change is expected with the last version of this PR.

@erikmd
Copy link
Collaborator Author

erikmd commented Aug 12, 2019

Thanks to the CI and its demo-repository (in which I had thus added one example of [%funty: …]), I realize there's now an issue with this [%funty: …] construct brought by the PR… Sorry for that, will investigate.

@erikmd erikmd force-pushed the impl-gh50-ppx-prot branch from 2c6d76b to efbbab0 Compare August 12, 2019 21:39
@erikmd
Copy link
Collaborator Author

erikmd commented Aug 12, 2019

The issue is now solved (to sum up, it was due to a typo in the ppx code, and a type issue related to the include Fun_ty line). The CI should now be green…

@yurug can you take a look at the current version?

@erikmd erikmd changed the title Extend ppx_metaquot with an extension point [%prot: _ -> _] Extend ppx_metaquot with an extension point [%funty: _ -> _] Aug 12, 2019
@erikmd erikmd force-pushed the impl-gh50-ppx-prot branch from efbbab0 to a04d3f9 Compare August 15, 2019 18:14
@erikmd erikmd force-pushed the impl-gh50-ppx-prot branch 2 times, most recently from 0785d1a to a880dc3 Compare August 18, 2019 22:57
@yurug
Copy link
Collaborator

yurug commented Sep 4, 2019

If you fix the conflicting files, I integrate this PR. Thank you for this contribution!
CC @erikmd

* Note: keep these two GADTs abstract.
* This is a first step towards implementing [%prot: int -> int].
* test_lib.mli: Move notations (!!), (@:) to fun_ty.mli;
  Put some [include (module type of Fun_ty)] instead.

* Therefore: no need for [open Fun_ty] in existing n-ary graders code.

* Add a deprecated synonym so that the renaming of Test_lib.ty_of_prot
  to Test_lib.ty_of_fun_ty yields no backward-compatibility issue.
@erikmd erikmd force-pushed the impl-gh50-ppx-prot branch from a880dc3 to c08394b Compare September 4, 2019 09:22
@erikmd
Copy link
Collaborator Author

erikmd commented Sep 15, 2019

Hi @yurug,
I am unsure if you received an automatic notification when I force-pushed this PR… so I confirm that the conflict that occurred in #302 is resolved.
Have a nice week!
Erik

@erikmd
Copy link
Collaborator Author

erikmd commented Sep 19, 2019

@yurug friendly ping :-)

@yurug yurug merged commit bbfc1b9 into ocaml-sf:master Oct 2, 2019
@erikmd erikmd deleted the impl-gh50-ppx-prot branch October 2, 2019 12:58
erikmd added a commit to pfitaxel/learn-ocaml that referenced this pull request Feb 1, 2020
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.

Question: construct a prot term from the string representation of the type

2 participants