implement new syntax for type declaration

[tatchi]

This is my attempt at implementing the new syntax as proposed in https://github.com/ocaml-ppx/ppx_import/issues/62.

It only changes the type declaration and not the module type declaration as the latter is already defined as a regular context-free rule. However, and for consistency purposes, I think that it would make sense to also change the syntax of the module type declaration. So

module type Hashable = [%import: (module Hashtbl.HashedType)]

would become

[%%import: module type Hashable = Hashtbl.HashedType] or module type%import Hashable = Hashtbl.HashedType

I've some local changes that partially address that but if you agree, I would prefer to address that in a separate PR as it seems trickier/requires more changes to the actual implementation.

[pitag-ha]

Thanks a lot for this PR @tatchi! From a ppxlib perspective and, as far as I can tell, also from a ppx_import perspective, the new syntax is a great improvement!

@ejgallego, will you have time to review this? There's no rush! I'm just checking in case you were expecting me to review or similar.

[ejgallego]

I can review this, but of course any help would be very welcome!

[ejgallego]

While I get cycles to look at this, adding a CHANGES entry would help @tatchi

[tatchi]

While I get cycles to look at this, adding a CHANGES entry would help @tatchi

I added a change entry. We should probably also adapt the examples in the readme. What syntax do you want to emphasize? [%%import ... ] or type%import ... ?

`

[ejgallego]

What would be the plan for this, should we start a 2.0 branch?

If so, should we allow ppx_import 1.x to be coinstallable with 2.x ?

[tatchi]

Hey, I forgot to mention but I rebased on top of master branch now that #69 is merged.

What would be the plan for this, should we start a 2.0 branch?

I'm not the one who can decide that, but IMO it would make sense yes.

If so, should we allow ppx_import 1.x to be coinstallable with 2.x ?

I have no idea 😅 cc @pitag-ha do you have any thoughts?

[ejgallego]

Thanks @tatchi , no idea either on what would be the best path for the 2.0 branch, I cc @gasche and @kit-ty-kate in case they'd like to weight in.

[pitag-ha]

Sorry for the very long time without answering!

We should probably also adapt the examples in the readme.

Yes, I agree.

What syntax do you want to emphasize? [%%import ... ] or type%import ... ?

My opinion on this is that type%import is the nicer syntax. However, of course, also this is up to the maintainers!

[pitag-ha]

should we allow ppx_import 1.x to be coinstallable with 2.x ?

@ejgallego, that's a good question, and I'm probably not the right one to answer this. My naive way of seeing this is that it's both simpler and better if they're not co-installable. Better because that way the ecosystem is likely going to switch faster to 2.x once the first packages have started doing so.

[pitag-ha]

I also have another remark. @pitag-ha mentioned the improvement in performance, due to not having to do another pass. Unfortunately, since ppx_import has to be run as a staged_pps, its pass won't be merged with other ppx (apart from the staged ones)...

Oh, right!

[tatchi]

Thanks for the careful review @panglesd 🙏

• A test for [%%import type t = Location.t] (without the :) should be added

I adapted one of the tests to this syntax, and also one with type%import ..... See https://github.com/ocaml-ppx/ppx_import/pull/68/commits/e51599245f9ce89d7682fdd4268ac7229c7b18bc

• A suggestion for supporting type%import t = Location.t and t2 = Location.t2

Thanks, that definitely makes sense and seems easy to support 😁. I made the changes in https://github.com/ocaml-ppx/ppx_import/pull/68/commits/19b230e68527f2f2984dd3580095f4ac82d0c306

• An unrelated suggestion for error reporting!

Interesting, I'll read the doc and see if I can avoid raising errors (that will be for another PR)

[ejgallego]

Thanks for all the work folks, so I guess it is time to create a 1.x branch and start thinking about merging this in master.

I agree that having a 2.0 release with the same package name is simpler, however the opam repos would need a pretty large patch to introduce a < 2.0 constraint in all of ppx_import rev deps.

Is this the standard operating procedure?

[kit-ty-kate]

Is this the standard operating procedure?

yes, it is totally normal and should be merged rather quickly. In this case you can even use opam admin add-constraint ppx_import<2.0 unconditionally given (if i understand correctly) everything is going to break

[ejgallego]

Thanks for the feedback @kit-ty-kate !

So I'm happy to go ahead with that then, @tatchi what's the status of this PR from your part?

[tatchi]

So I'm happy to go ahead with that then, @tatchi what's the status of this PR from your part?

I should have addressed all the points @panglesd mentioned in https://github.com/ocaml-ppx/ppx_import/pull/68#pullrequestreview-1088712206. Except for the improvement of the errors for which I opened a separate draft PR https://github.com/ocaml-ppx/ppx_import/pull/73

There's also the README that needs to be adapted, but I believe we can do it in a separate PR too.

[ejgallego]

Great! I think then this is ready to go, I'll do a last review pass and some internal testing ASAP.

[tatchi]

I am not sure however if we should merge this PR without an update to the README; personally I'd prefer to keep the doc in sync at all times.

I updated the readme with the new syntax. Note that the module type declaration syntax hasn't been modified (because it's already written as a context-free rule).

IMO, it would make sense to update it to module type%import ... to stay consistent. Maybe something to keep in mind before releasing a 2.0 ?

[ejgallego]

IMO, it would make sense to update it to module type%import ... to stay consistent. Maybe something to keep in mind before releasing a 2.0 ?

Yes! If you think it is a good idea to keep track of this, please open an issue and assign the 2.0 milestone to it.

[ejgallego]

Thanks a lot for all the work, I've created the v1.x branch for patches going to the 1.x series.