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

Make deriver #1

Open
wants to merge 50 commits into
base: main
Choose a base branch
from
Open

Make deriver #1

wants to merge 50 commits into from

Conversation

ayc9
Copy link

@ayc9 ayc9 commented Mar 8, 2022

This is an implementation of the make PPX deriver, based on previous implementations and updated using latest ppxlib API. The deriver works on record types, and supports @main and @default annotations on specific fields within the record type. Usage is documented in the readme. Special attention is needed for:

  • General code structure and readability (changes to function names, refactors welcome)
  • Testing coverage (especially for cases with colluding features, like @main on an option type)

Thank you!

@ayc9 ayc9 marked this pull request as ready for review March 17, 2022 21:29
Copy link
Member

@pitag-ha pitag-ha 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 for this implementation @ayc9 , it looks great! And sorry for the delay in reviewing (as you know, I was on vacation).

I only have one important question: why is this still marked as a draft? :) I think it's ready/very close to being ready to be merged! I've only looked over the first part so far and have left some minor comments; all of that is just nitpicking and nothing is important/blocking (in fact, you don't even need to take the suggestions if you don't agree).

Just one thing that I find a bit more important: It would be good to add something like "deriver make:" at the beginning of each error message. For example "deriver make: "We cannot expose functions that explicitly create private records.". That's just to make clear that the error doesn't come from the compiler, but from the deriver, when the user encounters it. Btw, it looks really good how you've embedded the errors!

P.S.: One thing that has called my attention: if I understand correctly, find_main does not only separate the arguments into the main argument and the other arguments but also turns around the order of the other arguments (which is needed later). Is that right? If so, it might be nice to somehow make that clear in the code to avoid confusion. I might come back to that later.

src/make/ppx_make.ml Outdated Show resolved Hide resolved
src/make/ppx_make.ml Outdated Show resolved Hide resolved
src/make/ppx_make.ml Outdated Show resolved Hide resolved
src/make/ppx_make.ml Show resolved Hide resolved
src/make/ppx_make.ml Outdated Show resolved Hide resolved
src/make/ppx_make.ml Outdated Show resolved Hide resolved
src/make/ppx_make.ml Outdated Show resolved Hide resolved
src/make/ppx_make.ml Outdated Show resolved Hide resolved
src/make/ppx_make.ml Show resolved Hide resolved
Copy link
Member

@pitag-ha pitag-ha left a comment

Choose a reason for hiding this comment

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

Hey @ayc9 , once again: thanks for this great PR! I've just had a look at the tests as well and they all also look good, so I'm already approving the PR. There's one kind of test that might be worth adding: records with type parameters, i.e. something along the lines

type 'a t = {a: 'a option } [@@deriving make]

but like all the other comments, that's nothing important/blocking. Let me know if you want to look into some of the comments or if you want me to merge like this :)

test/make/signature.t Outdated Show resolved Hide resolved
test/make/signature.t Outdated Show resolved Hide resolved
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