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

Remove ppx jane from prelude #3313

Merged
merged 11 commits into from
May 5, 2020
Merged

Remove ppx jane from prelude #3313

merged 11 commits into from
May 5, 2020

Conversation

yminsky
Copy link
Member

@yminsky yminsky commented May 4, 2020

Remove ppx_jane and open Base from a bunch of prelude.ml files. This required a bunch of follow-on changes to merge env's together, and to add some #require's to explicitly turn on syntax extensions. This all seems good.

There are some weird corners, notably, for some reason ppx_sexp_conv doesn't seem to work when #require'd, but ppx_jane does. I posted on issue there, and will fix this when that is resolved:

janestreet/ppx_sexp_conv#30

@yminsky
Copy link
Member Author

yminsky commented May 4, 2020

Note that there's a bunch of whitespace-only changes here that for some reason Github is incompetent at hiding, even when you turn on whitespace hiding. If you look at the diff in Patdiff, everything looks fine.

@avsm
Copy link
Member

avsm commented May 5, 2020

@NathanReb there's a strange anomaly in environment variable handling.

If I run this PR with

env TZ=UTC dune runtest

...it passes the expect test.

However, forcing the env-var in either dune-workspace with:

(lang dune 2.0)
(env (_ (env-vars (TZ UTC))))

fails to force it, and the expect tests fail.

The minimal place to force the env-var to be set would be in the dune file for the README.md, but the mdx invocation doesn't allow an environment variable to be set. Would you be able to investigate this one?

@NathanReb
Copy link
Collaborator

Have you tried setting it with MDX labels? It should work and might be a good work around until we figured this out!

@NathanReb
Copy link
Collaborator

I.e. adding ,set-TZ=UTC to the block opening line should work. There's a high chance these labels are ignored for toplevel blocks but if so, that's a bug and we should fix it!

@yminsky
Copy link
Member Author

yminsky commented May 5, 2020

For now, I've added non-determinism markers. We can remove them when we have a solution.

@yminsky yminsky merged commit 39b3ef7 into master May 5, 2020
@yminsky yminsky deleted the remove-ppx_jane-from-prelude branch May 5, 2020 14:43
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.

3 participants