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

Add open to expose yojson functions used in derives #201

Merged
merged 2 commits into from
Aug 11, 2023

Conversation

mt-caret
Copy link
Contributor

@mt-caret mt-caret commented Aug 8, 2023

Fixes #200.

I tried running some tests as best as I can but I'm not sure if the test failures I'm getting are relevant to #200.

@mt-caret mt-caret marked this pull request as ready for review August 8, 2023 09:56
@akabe
Copy link
Owner

akabe commented Aug 9, 2023

@mt-caret
First and foremost, let me express our gratitude for your donation. It means a lot to me and keeps me motivated to continue our work.

Also, I wanted to say a thank you for helping me fix the problem that I had left untouched for years.
Your patch looks good, but an integration test related to ppx_yojson_conv fails:

2023-08-09T08:39:07 [INFO] SEND: HMAC=d07f878740bfba2dcea2c2a4dc741c3eaafd5358e3883bcc6a4682def75c80e2; header={"msg_id":"3e80dbb2-de6d-4ca0-a909-530860a8eb3e","msg_type":"error","session":"6f36d72f-bf78e6d077b8440405e80146","date":"2023-08-09T08:39:07.8911Z","username":"runner","version":"5.3"}; parent={"msg_id":"6f36d72f-bf78e6d077b8440405e80146_27949_2","msg_type":"execute_request","session":"6f36d72f-bf78e6d077b8440405e80146","date":"2023-08-09T08:39:07.770278Z","username":"runner","version":"5.3"}; content={"ename":"error","evalue":"compile_error","traceback":["File \"[1]\", line 3, characters 17-20:\n3 | type t = { foo : int; baz : string; } [@@deriving yojson]\n                     ^^^\nError: Unbound value int_of_yojson\n"]}; metadata={}

cf. https://github.com/akabe/ocaml-jupyter/actions/runs/5795450506/job/15740046501?pr=201#step:13:213

Could you add open Ppx_yojson_conv_lib.Yojson_conv.Primitives to the file tests/integration/ppx.ipynb?

@akabe akabe self-requested a review August 9, 2023 08:54
@mt-caret
Copy link
Contributor Author

mt-caret commented Aug 9, 2023

First and foremost, let me express our gratitude for your donation. It means a lot to me and keeps me motivated to continue our work.

Thank you for working on this amazing project; I'm impressed by how well this works, and sponsoring your work is the very least I can do :)

Could you add open Ppx_yojson_conv_lib.Yojson_conv.Primitives to the file tests/integration/ppx.ipynb?

Done!

@akabe
Copy link
Owner

akabe commented Aug 11, 2023

Thank you so much!

@akabe akabe merged commit e8d7fbd into akabe:master Aug 11, 2023
5 checks passed
@akabe
Copy link
Owner

akabe commented Aug 11, 2023

@mt-caret
In case you're interested, I have the option to add your icon and a web page link to the README#Sponsors. Please let me know your thoughts on how you'd like to proceed.

@mt-caret
Copy link
Contributor Author

Thanks so much for the offer, but no thanks; my sponsorships are all private, and I'm not very interested in advertising my sponsorships :)

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.

opam install error
2 participants