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 the culprit json fragment in the error messages systematically. #47

Merged
merged 8 commits into from
Dec 20, 2024

Conversation

EmileTrotignon
Copy link
Contributor

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.

🤘🏼

ppx/test/errors.t/run.t Outdated Show resolved Hide resolved
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!

ppx/test/errors.t/run.t Outdated Show resolved Hide resolved
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.

ppx/browser/ppx_deriving_json_js.ml Outdated Show resolved Hide resolved
ppx/browser/ppx_deriving_json_runtime.ml Outdated Show resolved Hide resolved
ppx/browser/ppx_deriving_json_runtime.ml Outdated Show resolved Hide resolved
ppx/browser/ppx_deriving_json_runtime.ml Outdated Show resolved Hide resolved
ppx/native/ppx_deriving_json_runtime.ml Outdated Show resolved Hide resolved
ppx/native/ppx_deriving_json_runtime.ml Outdated Show resolved Hide resolved
ppx/native/ppx_deriving_json_runtime.ml Outdated Show resolved Hide resolved
ppx/browser/ppx_deriving_json_runtime.ml Outdated Show resolved Hide resolved
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
Contributor 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
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.

4 participants