Skip to content

Conversation

kit-ty-kate
Copy link
Collaborator

No description provided.

@kit-ty-kate
Copy link
Collaborator Author

Supersedes #30

Copy link
Contributor

@gasche gasche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot! I left two small comments for minor changes that would improve the readability of the result, but this is a very nice change.

@kit-ty-kate
Copy link
Collaborator Author

Requires ocaml-ppx/ppx_deriving#248

@gasche
Copy link
Contributor

gasche commented Dec 11, 2020

Out of curiosity, can you show the place requires this fix, by using lid_of_string with a dotted-path string?

@kit-ty-kate
Copy link
Collaborator Author

Sorry for the lack of reply. Here is the test failure I get with plain unpatched ppx_deriving.5.2:

File "src_test/protoc_inner.ml", lines 1-2, characters 0-32:
1 | type foo = int
2 | [@@deriving protobuf { protoc }]
Error: This variant pattern is expected to have type Protobuf.payload_kind
       The constructor Protobuf.Varint does not belong to type Protobuf.payload_kind
File "src_test/test_syntax.ml", line 15, characters 0-35:
15 | type b = bool [@@deriving protobuf]
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Error: This variant pattern is expected to have type Protobuf.payload_kind
       The constructor Protobuf.Varint does not belong to type Protobuf.payload_kind

The Protobuf.Varint string "parsed" as one indent and not Ldot ("Protobuf", Lident "Varint") comes from string_payload_kind_of_pb_encoding and is passed to Ppx_deriving.Ast_conveinance.lid_of_string through Ppx_deriving.Ast_conveinance.pconstr.

This function has type val pconstr : string -> pattern list -> pattern so one would expect to be able to pass names including its full module path.

@kit-ty-kate
Copy link
Collaborator Author

This PR is now ready to be merged (I don't have write access on this one)

@gasche
Copy link
Contributor

gasche commented Feb 4, 2021

Thanks! The CI fails on 4.03 and 4.04, and you added a bound (>= 4.05) anyway. Could you change the CI setup to stop testing <4.05?

@kit-ty-kate
Copy link
Collaborator Author

Should we simply remove Travis-CI entirely since OCaml-CI is already a superset of Travis?

@kit-ty-kate
Copy link
Collaborator Author

CI is green now.

@gasche
Copy link
Contributor

gasche commented Feb 4, 2021

The reasoning is that travis-CI was unnecessary because ocaml-ci covers the same checks. Is that correct?

How is the ocaml-ci configured? I couldn't find a configuration file in the repository. (I thought that the tool is still experimental and people need to be whitelisted to use it, does it work because ocaml-ppx is whitelisted?)

@kit-ty-kate
Copy link
Collaborator Author

The reasoning is that travis-CI was unnecessary because ocaml-ci covers the same checks. Is that correct?

Yes

How is the ocaml-ci configured? I couldn't find a configuration file in the repository. (I thought that the tool is still experimental and people need to be whitelisted to use it, does it work because ocaml-ppx is whitelisted?)

It's a no-configuration CI. To be eligible for it, the project must be a standard dune project. Internally it calls dune build @install @runtest and uses the opam file to install the dependencies.

@gasche gasche merged commit 5992469 into ocaml-ppx:master Feb 4, 2021
@gasche
Copy link
Contributor

gasche commented Feb 4, 2021

Merged, thanks for the extra explanation. I guess this needs a release now?

@kit-ty-kate
Copy link
Collaborator Author

that'd be perfect yes (i can do it for you if you don't have the time, though I'd need write access)

@gasche
Copy link
Contributor

gasche commented Feb 4, 2021

Thanks! I just sent you an invitation for maintenance access.

@kit-ty-kate kit-ty-kate mentioned this pull request Feb 4, 2021
kit-ty-kate added a commit to kit-ty-kate/opam-repository that referenced this pull request Feb 4, 2021
CHANGES:

  * Add support for OCaml 4.11 (ocaml-ppx/ppx_deriving_protobuf#36)
    (Thierry Martinez, review by Gabriel Scherer)
  * Add support for OCaml 4.12 (ocaml-ppx/ppx_deriving_protobuf#39)
    (Kate Deplaix, review by Gabriel Scherer)
  * Port to ppx_deriving 5.0 and ppxlib (ocaml-ppx/ppx_deriving_protobuf#39)
    (Kate Deplaix, review by Gabriel Scherer)
  * Upgrade the tests from ounit to ounit2 (ocaml-ppx/ppx_deriving_protobuf#39)
    (Kate Deplaix, review by Gabriel Scherer)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants