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

Migrate to ppxlib >= 0.18.0 #5

Merged
merged 8 commits into from
May 4, 2021
Merged

Migrate to ppxlib >= 0.18.0 #5

merged 8 commits into from
May 4, 2021

Conversation

giltho
Copy link
Contributor

@giltho giltho commented Nov 21, 2020

I'm using flow_parser as a dependency for a JavaScript analysis tool, I'm trying to upgrade ocaml but I need newer versions of ppxs all over. Flow_parser is blocking right now because of ppx_gen_rec.
Not merging this wouldn't be blocking for me, I just thought I'd submit it to help a bit

fixes ocaml-ppx/ppxlib#142

@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Nov 21, 2020
@giltho
Copy link
Contributor Author

giltho commented Nov 21, 2020

There seem to be an issue when using this with ppx_deriving though..
ppx_deriving has its effect at the same time, it unrolls the [@@deriving show] into val show... and ppx_gen_rec raises
"Psig_value not supported yet".
For now, this can't be used in flow_parser...

@mroch
Copy link
Contributor

mroch commented Nov 21, 2020

thanks! i'm on vacation for thanksgiving next week so it'll take a bit to check it out in detail but I skimmed this on my phone.

this is intended to run before deriving, so that the [@@deriving show] in the sig gets copied to the value, and then both are expanded separately. my understanding of ppxlib is that it replaces extensions top down, so it's interesting that this doesn't just work.

hopefully there's a way to make ppxlib work. if not, maybe just upgrading ocaml-migrate-parsetree resolves the version constraints you're running into?

alternatively, solving #3 so we don't need this at all could be a last resort

@mroch
Copy link
Contributor

mroch commented Nov 21, 2020

#2 fixed this same issue in ocaml-migrate-parsetree, but we don't have the same position option in ppxlib

@giltho
Copy link
Contributor Author

giltho commented Nov 21, 2020

No worry ! It gives me a bit of time to think about it. Very busy with coursework marking this week end :/
Thanks for your feedback

@giltho
Copy link
Contributor Author

giltho commented Nov 26, 2020

I must have done something wrong when I tested ppx_gen_rec locally the first time, because I just tested it again and actually there's no conflict with ppx_deriving 🤔
To make sure of that, I've added the corresponding test and a CI that executes it

@giltho
Copy link
Contributor Author

giltho commented Jan 12, 2021

Hello and Happy new year !

Any update on that side ? Anything I should do differently ?
I'm just checking up on the state of my current PRs, no emergency at all of course.

@kit-ty-kate
Copy link

Is there any news about this change by any chance?

@mroch
Copy link
Contributor

mroch commented May 4, 2021

we've been testing this out for a while internally on flow and it seems to work with ppx_deriving. i'll merge!

@mroch mroch merged commit 4a2f022 into flow:master May 4, 2021
@mroch
Copy link
Contributor

mroch commented May 4, 2021

now i have to remember how to do a release 😂

@giltho
Copy link
Contributor Author

giltho commented May 6, 2021

Let me know if I can help in any way in publishing / syncing flow with this version :)
I'm quite excited to be able to throw my fork of flow away and use flow_parser directly at the source

@mroch
Copy link
Contributor

mroch commented May 6, 2021

published v2.0.0! thanks again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Do not delete this pull request or issue due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port ppx_gen_rec to ppxlib
4 participants