Conversation
rprimet
force-pushed
the
web-interpreter
branch
2 times, most recently
from
83687b2 to
cb369c7
Compare
denismerigoux
left a comment
denismerigoux
left a comment
There was a problem hiding this comment.
OK for me, the overlap with #954 is very small and we should merge this first.
rprimet
force-pushed
the
web-interpreter
branch
from
8e2b76a to
aae5116
Compare
rprimet
force-pushed
the
web-interpreter
branch
from
aae5116 to
2999296
Compare
AltGr
left a comment
AltGr
left a comment
There was a problem hiding this comment.
Nice work ! I have a few nitpicks, as always, but this feels pretty solid.
compiler/web/dune
Outdated
| (:standard -linkall)) | ||
| (js_of_ocaml | ||
| (flags | ||
| (:standard --effects=cps))) |
There was a problem hiding this comment.
I am curious why the flag, I don't believe we make any use of effects at the moment ?
There was a problem hiding this comment.
That was due to a build warning:
Warning [missing-effects-backend]: your program contains effect handlers; you
should probably run js_of_ocaml with option '--effects=cps'
I guess this implies that effects are being used by some library?
| "zarith_stubs_js" {>= "v0.17.0"} | ||
| "js_of_ocaml" {>= "6.0"} | ||
| "js_of_ocaml-ppx" {>= "6.0"} |
There was a problem hiding this comment.
Should we make these optional dependencies ? (Open question...)
There was a problem hiding this comment.
I'll defer to you on this!
There was a problem hiding this comment.
Thanks for adding these tests. I'm a little worried about their maintenance though; ideally at some point we should be able to run on the scope tests in tests/ ; although this can only cover "positive" cases, not verify proper error cases.
Another solution would be to turn them into catala-test-cli, but that would require a liitle bit of additional support for clerk runtest to be able to run node catala_web_interpreter.js, feed it with the program as input and retrieve the output.
There was a problem hiding this comment.
I agree with your remark about the maintenance burden however these tests also exercise behaviors that are specific to the web version (e.g. the web interpreter API, error position reporting, user-supplied multiple files) and were sometimes written after uncovering a bug, so that may be reasonable?
There is now a loop for running (a subset of) scope tests in tests/ ; note that doing this however required a few changes in messages.ml / mli (because of unix / jsoo behavior differences) so a further review would be useful ; the nice thing is that we now surface warnings in the JS API which will help displaying them nicely in the playground editor (previously warnings were disabled and only errors were reported)
rprimet
force-pushed
the
web-interpreter
branch
from
58acde8 to
66fd36b
Compare
Co-Authored-By: Louis Gesbert <louis.gesbert@inria.fr>
rprimet
force-pushed
the
web-interpreter
branch
from
04c49ee to
a09cd4a
Compare
|
@AltGr Thanks for the nice and thorough review! |
rprimet
force-pushed
the
web-interpreter
branch
3 times, most recently
from
7bfe209 to
e49265d
Compare
rprimet
force-pushed
the
web-interpreter
branch
from
e49265d to
30a578e
Compare
rprimet
force-pushed
the
web-interpreter
branch
from
74c0c11 to
350c73b
Compare
github-project-automation
bot
moved this from In Progress
to Done
in Catala - language & tooling
3 participants
Supersedes #952 in part (other PR to come in catala-book for the web playground)