search

ocaml-ppx / ppx_import

Less redundancy in type declarations and signatures
MIT License
89 stars 28 forks source link

[ci] [opam] Bump OCaml upper bound to 4.12.0 #53

Closed ejgallego closed 3 years ago

ejgallego commented 3 years ago

ppx_import seems to work in 4.12 without further changes.

We may want to update the AST at a later point.

A question is if we should do a new release for this change in the Opam file, but IMHO it would be more practical just to update opam-repos?

What do you think @gasche @kit-ty-kate ?

ejgallego commented 3 years ago

Actually there is a subtle problem in opam-repository, ppx_import 1.7.0 doesn't declare an upper bound, so it is installable under 4.12.0, however 1.8.0 is not. 1.7.0 seems to work fine too.

kit-ty-kate commented 3 years ago

yeah I had already removed the restriction before the previous release. I missed the new unnecessary upper-bound constraint in the latest release two weeks ago. I'll remove the constraint from opam-repo in 5 minutes

kit-ty-kate commented 3 years ago

https://github.com/ocaml/opam-repository/pull/18234

ejgallego commented 3 years ago

@kit-ty-kate fantastic thanks; do you suggest then we remove the constraint in the .opam file too?

kit-ty-kate commented 3 years ago

yeah I think it would be better to remove it. In my opinion upper-bound constraints on packages make sense for packages that explicitly require a range of versions (OMP, ppxlib, camlp4, camlp5, reason, merlin, …) or if you know in advance something will/does break for sure. For pretty much everything else, it doesn't make much sense to have it in the dev repository

ejgallego commented 3 years ago

yeah I think it would be better to remove it. In my opinion upper-bound constraints on packages make sense for packages that explicitly require a range of versions (OMP, ppxlib, camlp4, camlp5, reason, merlin, …) or if you know in advance something will/does break for sure. For pretty much everything else, it doesn't make much sense to have it in the dev repository

Thanks for the feedback, adjusted!

gasche commented 3 years ago

If whatever variant of ppx_tools we use has a 4.12 release, we should also try to upgrade to that, so that 4.12-specific AST constructs are handled properly by ppx-import.

ejgallego commented 3 years ago

If whatever variant of ppx_tools we use has a 4.12 release, we should also try to upgrade to that, so that 4.12-specific AST constructs are handled properly by ppx-import.

Indeed that sounds good, would require a new release; I will merge this tweak for now and we can try to bump the AST version when we get a few free cycles.

ejgallego commented 3 years ago

I'm not sure if ppx_tools_versioned will support 4.12 soon, so if the plan for them is not to do so, we may need to solve #44 first.