implement new syntax for type declaration #68

tatchi · 2022-02-02T18:24:17Z

This is my attempt at implementing the new syntax as proposed in #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.

src_test/ppx_deriving/test_intf.ml

pitag-ha · 2022-02-28T13:13:20Z

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 · 2022-03-02T15:34:18Z

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

ejgallego · 2022-03-02T15:35:13Z

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

tatchi · 2022-04-14T21:00:41Z

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 · 2022-06-15T10:06:24Z

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 · 2022-06-27T21:29:55Z

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 · 2022-06-28T10:06:04Z

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 · 2022-08-25T16:05:51Z

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 · 2022-08-25T16:10:25Z

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.

panglesd

(I'm commenting this as a ppxlib maintainer)

Looks good!

My comments in code summed up here are:

A test for [%%import type t = Location.t] (without the :) should be added
A suggestion for supporting type%import t = Location.t and t2 = Location.t2
An unrelated suggestion for error reporting!

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)...

Finally, the README should be modified, but I understand this modification cannot be merged until the new version is released (or, in a 2.0 branch). In this README, I think it should be emphasized that the [@@deriving] attribute should be put inside the [%%import]!

src/ppx_import.ml

src_test/ppx_deriving/test_intf.ml

panglesd · 2022-08-29T14:17:04Z

src/ppx_import.ml

+              Location.raise_errorf ~loc
+                "[%%import] cannot import a functor application %s"
+                (string_of_lid lid)


Again, this comment is not really related to the PR, but it is a bad practice to raise errors with ppxlib, as it prevents other rewriters to work and more importantly it prevents merlin to work...
Instead, errors should be embedded. I have written a section in the ppxlib manual about his here, which includes a guide for migrating from raising to embedding.

I understand that this should most certainly be part of another PR!

pitag-ha · 2022-08-30T11:20:52Z

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 · 2022-08-30T12:29:20Z

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 e515992

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 19b230e

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 · 2022-08-30T17:39:46Z

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 · 2022-08-31T11:31:22Z

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 · 2022-08-31T11:33:50Z

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 · 2022-08-31T14:20:25Z

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 #68 (review). Except for the improvement of the errors for which I opened a separate draft PR #73

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

ejgallego · 2022-09-01T13:49:23Z

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

ejgallego · 2022-09-01T13:50:57Z

CHANGES.md

+  (* or *)
+  type%import loc = Location.t
+  ```
+


Would it be possible to explain how the other parts of the syntax work? In particular the with foo := bar alias.

Maybe it is better just to update the readme in this PR too? Indeed, it could be a good idea to keep the documentation of the code in sync.

ejgallego

LGTM, thanks a lot!

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 will proceed to setup the right branches and opam repository constraints , then merge this PR.

tatchi · 2022-09-03T04:59:41Z

README.md

@@ -35,7 +35,7 @@ Syntax
 For example:

 ``` ocaml
-# type loc = [%import: Location.t];;
+# type%import loc = Location.t;;
 type loc = Location.t = { loc_start : Lexing.position; loc_end : Lexing.position; loc_ghost : bool; }
 # module type Hashable = [%import: (module Hashtbl.HashedType)];;


As I mentioned in #68 (comment), that would be nice to change the syntax of module type declaration too.

Happy to open a follow-up PR to address that :)

tatchi · 2022-09-03T05:04:06Z

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 · 2022-09-08T15:47:37Z

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 · 2022-09-08T15:51:23Z

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

tatchi commented Feb 2, 2022

View reviewed changes

src_test/ppx_deriving/test_intf.ml Show resolved Hide resolved

ejgallego self-assigned this Feb 28, 2022

ejgallego mentioned this pull request Apr 15, 2022

Ppxlib.0.26.0 compatibility #69

Merged

tatchi added 2 commits June 21, 2022 07:15

implement new syntax

47e5cb0

add changes entry

546d799

tatchi force-pushed the impl-new-ppx-syntax branch from 5f93803 to 546d799 Compare June 21, 2022 05:28

panglesd approved these changes Aug 29, 2022

View reviewed changes

tatchi added 2 commits August 30, 2022 11:25

cover alternate syntaxes in tests

e515992

add support for type%import t = a and t2 = b

19b230e

tatchi mentioned this pull request Aug 30, 2022

Avoid raising errors #73

Merged

ejgallego added this to the 2.0 milestone Sep 1, 2022

ejgallego added the enhancement label Sep 1, 2022

ejgallego reviewed Sep 1, 2022

View reviewed changes

ejgallego approved these changes Sep 1, 2022

View reviewed changes

update readme

5994950

tatchi commented Sep 3, 2022

View reviewed changes

ejgallego merged commit c9df42c into ocaml-ppx:master Sep 8, 2022

tatchi mentioned this pull request Sep 9, 2022

Update the module type syntax #74

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement new syntax for type declaration #68

implement new syntax for type declaration #68

tatchi commented Feb 2, 2022 •

edited

Loading

pitag-ha commented Feb 28, 2022

ejgallego commented Mar 2, 2022

ejgallego commented Mar 2, 2022 •

edited

Loading

tatchi commented Apr 14, 2022 •

edited

Loading

ejgallego commented Jun 15, 2022

tatchi commented Jun 27, 2022

ejgallego commented Jun 28, 2022

pitag-ha commented Aug 25, 2022

pitag-ha commented Aug 25, 2022

panglesd left a comment •

edited

Loading

panglesd Aug 29, 2022

pitag-ha commented Aug 30, 2022

tatchi commented Aug 30, 2022

ejgallego commented Aug 30, 2022

kit-ty-kate commented Aug 31, 2022

ejgallego commented Aug 31, 2022

tatchi commented Aug 31, 2022

ejgallego commented Sep 1, 2022

ejgallego Sep 1, 2022

ejgallego left a comment •

edited

Loading

tatchi Sep 3, 2022

tatchi Sep 3, 2022 •

edited

Loading

tatchi commented Sep 3, 2022

ejgallego commented Sep 8, 2022

ejgallego commented Sep 8, 2022

implement new syntax for type declaration #68

implement new syntax for type declaration #68

Conversation

tatchi commented Feb 2, 2022 • edited Loading

pitag-ha commented Feb 28, 2022

ejgallego commented Mar 2, 2022

ejgallego commented Mar 2, 2022 • edited Loading

tatchi commented Apr 14, 2022 • edited Loading

ejgallego commented Jun 15, 2022

tatchi commented Jun 27, 2022

ejgallego commented Jun 28, 2022

pitag-ha commented Aug 25, 2022

pitag-ha commented Aug 25, 2022

panglesd left a comment • edited Loading

Choose a reason for hiding this comment

panglesd Aug 29, 2022

Choose a reason for hiding this comment

pitag-ha commented Aug 30, 2022

tatchi commented Aug 30, 2022

ejgallego commented Aug 30, 2022

kit-ty-kate commented Aug 31, 2022

ejgallego commented Aug 31, 2022

tatchi commented Aug 31, 2022

ejgallego commented Sep 1, 2022

ejgallego Sep 1, 2022

Choose a reason for hiding this comment

ejgallego left a comment • edited Loading

Choose a reason for hiding this comment

tatchi Sep 3, 2022

Choose a reason for hiding this comment

tatchi Sep 3, 2022 • edited Loading

Choose a reason for hiding this comment

tatchi commented Sep 3, 2022

ejgallego commented Sep 8, 2022

ejgallego commented Sep 8, 2022

tatchi commented Feb 2, 2022 •

edited

Loading

ejgallego commented Mar 2, 2022 •

edited

Loading

tatchi commented Apr 14, 2022 •

edited

Loading

panglesd left a comment •

edited

Loading

ejgallego left a comment •

edited

Loading

tatchi Sep 3, 2022 •

edited

Loading