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

Improve error messages #80

Open
NathanReb opened this issue Oct 30, 2018 · 3 comments
Open

Improve error messages #80

NathanReb opened this issue Oct 30, 2018 · 3 comments

Comments

@NathanReb
Copy link
Collaborator

It would be nice to improve the error reporting of the derived of_yojson functions as it would really ease debugging errors in the parsed json or in hand written parsers involved.

Some of them can be improved quite easily:

[%of_yojson: int] `Null

is

Error ""

I think in general it would also be a huge improvement to be able to track the error location through the json structure.

For example, if we consider the following:

type t =
  { a : A.t
  ; b : B.t
  }
  [@@deriving of_yojson]

it would be helpful to concatenate "in field a: " to the A.of_yojson error message when the parsing of a fails.

I don't have a clear idea of what would be the best way to handle those but would be happy to discuss it with you and to provide a PR if you feel like that should be improved.

@gasche
Copy link
Contributor

gasche commented Nov 3, 2018

The requirement/need/wish seems very sensible to me. Two remarks:

  • The error-reporting scheme should not hurt performance too much. I am highly confident that it's possible to have both at the same time (your idea of recursively passing an "access path" to the parser to report where the problem is seems mostly free), but I would expect any change to come with summary benchmarks to measure this.

  • ppx_deriving_yojson is supposed to desugar into straightforward calls to the Yojson library, not implement elaborate logic itself. Ideally we want all "interesting" logic to be implemented at the Yojson level, and simply used from the deriver. In the present case, however, I am not completely sure how to do this: error-reporting may require transporting information as we traverse data structures, so we need to do something on our end. I would still expect most of the logic to be added/improved at the Yojson level.

@whitequark
Copy link
Collaborator

(Agreed on both counts.)

@emillon
Copy link
Contributor

emillon commented Mar 19, 2019

Just improving error messages in these case (without adding location) is a nice quick win. I just got bit by [%of_yojson: string] returning Error "" and had no idea where to start :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants