-
Notifications
You must be signed in to change notification settings - Fork 16
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
Remove ppx generators #342
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One word: Awesome! 😄 🎉
should we remove also derivers in doc/?
I actually thought the examples were run in the CI - but I realize now that they are not.
Based on the above, I was thinking of using ppx_deriving
in the text (to keep it short) but including a commented show
etc in the code
(* Note: you can omit the following definition if you uncomment
(preprocess (pps ppx_deriving.show))
in the dune file *)`
Since we don't run the examples, I think it is fine to leave it as is though.
should we use (=) instead of those equal_ functions?
Polymorphic equality would certainly make things a bit shorter, but there are also arguments against using it entirely. I think what you have done is very reasonable 👍
we could improve the pretty-printers to get closer to standard OCaml printers and/or to the derived ones (opening boxes, etc.) The code is structured so that this should be easy to do.
That would be nice to have(TM) but not strictly needed ATM.
I also realize our expect outputs were already a bit inconsistent in we would print
(Remove -605797133) : Some (-605797133)
which is now
Remove -605797133 : Some (-605797133)
- where we have a constructor with a negative int
argument on either side of the colon.
This is just me getting distracted and doesn't mean anything for the PR.
A CHANGES entry to announce the new Util
sub-modules Pp
and Equal
would perhaps be in order...
Add a sh+awk script to help write the boilerplate code instead of relying on ppxlib-based derivers
Add Util submodules to define pretty-printers and equality testers
Generate all code once and for all, hand-tuning the boilerplate code generated by helper scripts
I thought so too, I expected them to be run at least in the documentation workflow, but I don’t think they are. Anyway, keeping the ppxlib dependency for examples that are run on current stable didn’t feel such a big deal.
👍
Well-spotted! The pretty-printers for numbers should be fixed now, and use parentheses when required for negative values.
I added that. Along those fixes, I also dropped the use of optional argument, as it was more cumbersome than I had hoped. The few cases where it allowed the implicit uses of the optional value were making it annoying when you define functions that must have that optional argument but they will ignore it (it is the case of pretty-printers for sum types when no constructor has an argument, for instance). I also added documentation to the |
Still LGTM 😃👍 |
CI summary:
None of these are related to the PR, so I'll merge 👍 |
Avoid using Seq.equal which was added in 4.14. Initially fixed in #340 and then broken in #342. Co-authored-by: Pau Ruiz Safont <[email protected]> Co-authored-by: Edwin Török <[email protected]>
Avoid using Seq.equal which was added in 4.14. Initially fixed in #340 and then broken in #342. Co-authored-by: Pau Ruiz Safont <[email protected]> Co-authored-by: Edwin Török <[email protected]>
This PR removes the ppx derivers for
show
,eq
andqcheck
.Open questions:
doc/
?(=)
instead of thoseequal_
functions?Closes #341.