Add Extension.V3.declare_with_path_arg #431
Conversation
There was a problem hiding this comment.
Thanks for the PR!
However, make test passes no matter what I do
This is I think because the test you modify is only enabled in OCaml between 4.08 and 4.11 (included). So if the OCaml version in your switch is outside these bounds, the test is not run.
However, the CI ran the test. You need to register the new transformation to another extension node.
50754f5 to
2eefd8f
Compare
|
Thanks for contributing, it's always nice to see new people on the repo! What's the use case you have in mind? In |
|
I was also puzzled by the fact that " |
Yes, but what's the use case for that? I'm assuming that the usual/intended use case for it in "V1" was to be able to pass in the module path manually given that the rewriter didn't have access to what we now call expansion context. I'm wondering what the use case of manually passing in a path argument is now that we do have the accurate |
|
Er, the "path arg" is, imho, badly named, because code-path now exists — I think out of two possible meanings of the word 'path', the current By contrast, the " e.g. in this example, fwiw, I think now would be a fantastic time to rename the wdyt? As for use-cases, who knows what all someone might overload that explicit "argument" to mean; but my usage (in ppx_trace) is effectively identical to that of
(This is a fantastic feature, and the primary reason we're still using |
|
Thanks for explaining the use case, sounds all good!
Sounds good as well. What name would you use? Btw, about the CI failures:
|
Signed-off-by: ELLIOTTCABLE <git@ell.io>
Signed-off-by: ELLIOTTCABLE <git@ell.io>
Signed-off-by: ELLIOTTCABLE <git@ell.io>
Signed-off-by: ELLIOTTCABLE <git@ell.io>
Signed-off-by: ELLIOTTCABLE <git@ell.io>
Signed-off-by: ELLIOTTCABLE <git@ell.io>
52e0c2f to
4fcc058
Compare
|
Okay, I chose, simply, "additional_arg" — although, yes, only a module-path will technically be accepted in this position, that's actually somewhat orthogonal? Let me know if that naming pleases you. I also added notes to the now-deprecated interfaces so people can find the new one. I forgot to ask — is there an appropriate test file that does get run on current versions? Somewhere better I can add tests for this? |
To be honest, I think it has more disadvantages than advantages:
So, I would be in favour of not changing the name.
I think the test is not run because of a change in the display of error (ocaml/ocaml#9657). But I don't think that's a good reason to not test on OCaml version So we should make this test run on all versions. One possibility is to manipulate the string returned by the expect test, but that's ad-hoc for each compiler change of displayed errors, and requires maintainance time that we don't have. |
This reverts commit 72ecdd7. Signed-off-by: ELLIOTTCABLE <git@ell.io>
Signed-off-by: ELLIOTTCABLE <git@ell.io>
Signed-off-by: ELLIOTTCABLE <git@ell.io>
|
Okay, I think your arguments hold water — wasn't too sure myself. Pushed a reversion of the renaming; and added 4.12+ tests, mirroring the approach elsewhere in the repo. :D Thanks for the hand-holding. ❤️ |
51dc448 to
f957173
Compare
There was a problem hiding this comment.
I finally had a look at the code, looks good to me! (unsurprisingly)
There are still some things to do to satisfy the CI, before merging... I can do them if you are tired of this!
- the
test/driver/transformations/test_412.mlfile should be added to the.ocamlformat-ignorefile (or a glob for all.mlfiles in this directory) - The 5.1 CI is still not happy with the test. We might need to add a
test_501.mlfile... (this method of having different tests for every breaking change in OCaml output is pretty annoying, but less than the other solutions in my opinion).
Signed-off-by: ELLIOTTCABLE <git@ell.io>
|
Okay! Take 5! 🤞 |
Signed-off-by: ELLIOTTCABLE <git@ell.io>
|
Thanks @ELLIOTTCABLE ! (and sorry for the delay...) |
|
You're totally fine, @panglesd ❤️ No pressure at all, just for my planning purposes — what's the general release-cadence likely to be for this to go up onto the public opam repo? We're already using this in production at Ahrefs (it's no big deal, we already have ... a lot of opam overrides 😅); but it's also blocking me being able to release |
|
Until now, mostly @pitag-ha has been taking care of the releases... She is currently on holiday. I think that with the release of ocaml 5.1, we will need to release a compatible version of ppxlib soon! So hopefully, you'll be able to release |
|
Yep, let's cut a release (as soon as we've merged the functor application migration fix). |
CHANGES: - Fix support for OCaml 5.1: migrated code preserves generative functor warnings, without creating more. Locations are better preserved. (ocaml-ppx/ppxlib#432, @pitag-ha, @panglesd) - Driver: Add `-unused-code-warnings` command-line flag. (ocaml-ppx/ppxlib#444, @ceastlund) - Add `?warning` flag to `Deriving.Generator.make`. (ocaml-ppx/ppxlib#440, @jacksonzou123 via @ceastlund) - Restore the "path_arg" functionality in the V3 API (ocaml-ppx/ppxlib#431, @ELLIOTTCABLE) - Expose migration/copying/etc. functions for all AST types needed by `Pprintast` (ocaml-ppx/ppxlib#454, @antalsz) - Preserve quoted attributes on antiquotes in metaquot (ocaml-ppx/ppxlib#441, @ncik-roberts) - Attribute namespaces: Fix semantics of reserving multi-component namespaces (ocaml-ppx/ppxlib#443, @ncik-roberts)
CHANGES: - Fix support for OCaml 5.1: migrated code preserves generative functor warnings, without creating more. Locations are better preserved. (ocaml-ppx/ppxlib#432, @pitag-ha, @panglesd) - Driver: Add `-unused-code-warnings` command-line flag. (ocaml-ppx/ppxlib#444, @ceastlund) - Add `?warning` flag to `Deriving.Generator.make`. (ocaml-ppx/ppxlib#440, @jacksonzou123 via @ceastlund) - Restore the "path_arg" functionality in the V3 API (ocaml-ppx/ppxlib#431, @ELLIOTTCABLE) - Expose migration/copying/etc. functions for all AST types needed by `Pprintast` (ocaml-ppx/ppxlib#454, @antalsz) - Preserve quoted attributes on antiquotes in metaquot (ocaml-ppx/ppxlib#441, @ncik-roberts) - Attribute namespaces: Fix semantics of reserving multi-component namespaces (ocaml-ppx/ppxlib#443, @ncik-roberts)
Seems like an oversight in the new API; came up in this Discuss thread.
makebuilds;make testpasses. However,make testpasses no matter what I do. Can't verify that my added test actually passes — but it's a trivial addition, it should be "if it compiles, it works." 🙈(Could use a
CONTRIBUTINGsection in the README with instructions as to how to build/test/PR, etc. hahaha.)