Migrate ocaml ppx context#478
Conversation
|
@Octachron it seems from our conversation during the last ppxlib dev meeting that the migration from 5.02 to 5.01 should include all load paths, both hidden and visible into the 5.01 The migration currently discards the hidden ones. I'll change that if you confirm! |
|
I think it would be better in term of backward compatibility: the idea of the hidden includes is to split the current list of |
357483b to
756429e
Compare
|
I changed the migration to merge visible and hidden when migrating down to 5.1! |
|
We should probably wait on the outcome of the following discussion to decide whether we should merge this into trunk-support: |
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> Co-authored-by: Antonio Nuno Monteiro <anmonteiro@gmail.com>
9010ee8 to
031dd11
Compare
|
We're going to go with this migration and keep the 5.2 changes on the OCaml side so we do need to get this merged! |
pitag-ha
left a comment
There was a problem hiding this comment.
Nice, thanks for both the implementation and the tests! The way the test works is a very good idea that makes a complicated test simple!
Apart from my one question about the location, it looks all good to me.
Btw, there's also a general question about what we'll want to do once we bump the AST to >=5.2. Once we do so, we could introduce a much simpler migration without track-keeping attr. We can discuss that once the time comes and I don't have a strong opinion, but I wanted to mention it already.
|
Btw, thanks, @anmonteiro! Every help we can get is always appreciated! A question out of curiosity: Have you been affected by the problem this PR solves? I'm asking because I know that in the past, some OCaml to JS compiler (concretely |
Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>
Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>
pitag-ha
left a comment
There was a problem hiding this comment.
Thanks, looks all good to me! 🚀
If you can still quickly address the version question below before merging, that's amazing, but merging sounds good to me already.
Signed-off-by: Nathan Rebours <nathan.p.rebours@gmail.com>
This PR adds migration of
ocaml.ppx.context's load_path between 5.1 and 5.2.No information is lost in the round-trip.
It comes with a test that should run only with the latest OCaml version, for now that's 5.2.
The test consists of adding a manually written
ocaml.ppx.contextto an ml file after an actual structure item so it is not discarded by the driver.The custom driver will look for this floating attribute, manually migrate it to 5.01 (any version before 5.02 would do since the format was stable before that point), and pretty print to stdout. It will then manually migrate it to the compiler version (i.e. at least 5.02) and pretty print again.
The driver actual output is discarded.
The only downside to this approach is that we have to re-implement a small part of pprintast for 5.01 in the custom driver but I'd say it's acceptable since we don't have to do it for any other version and it makes the cram test quite simple.
Let me know what you think!