Skip to content

Conversation

EmileTrotignon
Copy link
Collaborator

The culprit fragment is the piece of json that is wrong. It is printed with a special shallow printer, in order to keep the messages short and to the point.

I have included new tests.

A bit of code smell is the fact that my code relies on the fact that the current json being converted is always called x. Help is appreciated on that front, in a lot of cases I could not get the proper expression.

I will probably add a bit more tests tomorrow.

Copy link
Member

@davesnx davesnx left a comment

Choose a reason for hiding this comment

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

🤘🏼

Copy link
Member

@anmonteiro anmonteiro left a comment

Choose a reason for hiding this comment

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

this looks pretty great, nice work!

Copy link
Collaborator

@andreypopp andreypopp left a comment

Choose a reason for hiding this comment

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

This is a very good improvement!

Left some comments below.

Copy link
Collaborator

@andreypopp andreypopp left a comment

Choose a reason for hiding this comment

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

Thank you!

@EmileTrotignon
Copy link
Collaborator Author

I thought it was a bit confusing to have all the runtime files mixed with the ppx files, especially because I added a bunch, so I moved them to a separate folder.

If there is no issue with that, I have addressed every comment I got, I think its time to merge (I do not have the rights to do it).

@andreypopp andreypopp merged commit 10fae36 into melange-community:main Dec 20, 2024
[%expr
if Stdlib.( <> ) [%e len] [%e eint ~loc n] then
Ppx_deriving_json_runtime.of_json_error
Ppx_deriving_json_runtime.of_json_msg_error ~json:[%e x]
Copy link
Member

Choose a reason for hiding this comment

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

@EmileTrotignon This function doesn't take a json arg but somehow the tests passed. 🤔

Copy link
Member

Choose a reason for hiding this comment

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

fixed in 3fa710d and be46a4d

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks !

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.

5 participants