-
Notifications
You must be signed in to change notification settings - Fork 471
Document container setup for OCaml and dune #7899
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
base: return-marker-flags
Are you sure you want to change the base?
Document container setup for OCaml and dune #7899
Conversation
@codex: do make test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codex Review: Here are some suggestions.
Reply with @codex fix comments
to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
let test_rewrite_loses_return_flag _ctx = | ||
let source = "let f = x => g(x)\n" in | ||
let structure = parse_structure source in | ||
let body = extract_fun_body structure in | ||
OUnit.assert_bool "pre-rewrite body should be marked" | ||
(Res_return_marker.expression_is_return body); | ||
let rewritten = Ppx_entry.rewrite_implementation structure in | ||
let rewritten_body = extract_fun_body rewritten in | ||
OUnit.assert_bool "rewritten body currently loses its return flag" | ||
(not (Res_return_marker.expression_is_return rewritten_body)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P1] Invert return-flag assertion after rewriting
The new test_rewrite_loses_return_flag
intentionally asserts that Ppx_entry.rewrite_implementation
drops the return marker (assert_bool … (not (Res_return_marker.expression_is_return rewritten_body))
). Tests should encode the desired behaviour, i.e. the flag should be preserved after rewriting. As written, the suite now passes only when the bug exists and will fail once the bug is fixed, which both masks the regression and locks the incorrect behaviour in place.
Useful? React with 👍 / 👎.
Summary
Testing
|
make format && make checkformat | ||
``` | ||
|
||
## Container Tooling Setup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also have our dev container setup (for working in a container in VS Code) at https://github.com/rescript-lang/rescript/tree/master/.devcontainer.
Do we actually need two different Docker setups?
If so, then it would be great to have a Dockerfile for this one, too, so that the agent just needs to run docker build
in the correct directory.
I did not get this to go very far. Even instructing to install opam via apt-get, it always gets to the point where some fetch is blocked:
|
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68cb775741a0833193e0e6246b9b4b75