Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Package lens.1.2.5: Migration to ppxlib #18575

Merged
merged 5 commits into from
Apr 26, 2021
Merged

Conversation

pdonadeo
Copy link
Contributor

No description provided.

@camelus
Copy link
Contributor

camelus commented Apr 25, 2021

Commit: 8a8b8c6

@pdonadeo has posted 20 contributions.

☀️ All lint checks passed 8a8b8c6
  • These packages passed lint tests: lens.1.2.5

☀️ Installability check (+1)
  • new installable packages (1): lens.1.2.5

Comment on lines 14 to 16
"ppx_deriving" {< "5.0"}
"ppx_tools" {build}
"ppxfind" {build}
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure this is still the case? It seems to conflict with your changelog

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course you are right, I forgot to update the opam file. I'm going to submit again. @kit-ty-kate

@kit-ty-kate
Copy link
Member

@pdonadeo could you return the fix in the opam upstream? I'm not sure ppxlib should be tagged as {build} because i don't know if it has a runtime (or if not, will always have none).
Also constraints were missing. The switch to ppxlib occurred in ppx_deriving 5.0 and you're using features from ppxlib 0.14 so i added the missing constraints.

@kit-ty-kate
Copy link
Member

Also, I'm gonna put my ppx_deriving maintainer hat on for a second: the ppx_deriving API is deprecated and if you have time to switch it would be better to use ppxlib directly. See ocaml-ppx/ppx_deriving#250
If you don't have the time to do this, don't worry the API will still be there for a reasonable amount of time (until it gets prohibitively expensive to maintain or something like that)

@kit-ty-kate
Copy link
Member

Good to go now. Thanks!

@kit-ty-kate kit-ty-kate merged commit 56fed3f into ocaml:master Apr 26, 2021
@pdonadeo
Copy link
Contributor Author

Thanks to you @kit-ty-kate for your job 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants