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

5.3 effect syntax #552

Merged
merged 5 commits into from
Jan 30, 2025
Merged

5.3 effect syntax #552

merged 5 commits into from
Jan 30, 2025

Conversation

hhugo
Copy link
Collaborator

@hhugo hhugo commented Jan 13, 2025

No description provided.

@NathanReb
Copy link
Collaborator

I like the idea, if that sounds good to you I'll make it more aligned with our other migration specific extensions and add tests to turn it into a full PR.

@hhugo
Copy link
Collaborator Author

hhugo commented Jan 13, 2025

I like the idea, if that sounds good to you I'll make it more aligned with our other migration specific extensions and add tests to turn it into a full PR.

Feel free to take it from there.

@NathanReb
Copy link
Collaborator

I added a test and tweaked a couple simple things.

I still have a couple concerns though before we can merge this:

  1. it won't work if someone's using the driver's source output as it just straight up prints the internal AST version without running the migration back to the compiler's version
  2. I wonder how it interacts with deriving_inline and other correction generating ppx-es. My feeling is that it should be fine as long as one doesn't try to directly get the deriver to embed code that uses the effect syntax but it will likely result in the corrected output containing the extension otherwise.

I'll test this a bit more. If it turns out I was right, I think we could add a flag to the driver to force it to migrate the AST and use the compiler's printer to prevent that from happening in the few places where it could cause troubles.

@NathanReb NathanReb force-pushed the effect-syntax branch 2 times, most recently from a72b543 to bdbe1dc Compare January 20, 2025 13:21
@NathanReb NathanReb marked this pull request as ready for review January 28, 2025 13:47
@NathanReb NathanReb force-pushed the effect-syntax branch 2 times, most recently from 341c175 to 6825170 Compare January 28, 2025 16:56
NathanReb and others added 5 commits January 28, 2025 18:05
Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>
Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>
Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>
Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>
Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>
@NathanReb NathanReb merged commit 591b262 into main Jan 30, 2025
5 of 6 checks passed
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.

2 participants