-
Notifications
You must be signed in to change notification settings - Fork 2
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
ppx: consistent errors in runtime #28
base: main
Are you sure you want to change the base?
Conversation
The main thing we fix here is the consistent use of exceptions in native runtime, which used, before this commit either its own Of_json_error or yojson's Type_error. Then there's ``` val of_string : string -> t ``` which is not required by ppx but was introduced so that the runtime modules can function as minimal JSON interfaces usable both in native and browser environments (so called universal workflow). Before this commit the error behaviour was not specified, now we introduce `exception Of_json of string` in both native and browser and make `of_string` raise it in case of invalid JSON.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very good
let of_string s = Js.Json.parseExn s | ||
let of_json_error msg = raise @@ Json.Decode.DecodeError msg | ||
|
||
exception Json_error of string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
encode_error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, this is JSON parsing error, not encode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, miss-read the code. Parsing_error then? ^^
| Some jsexn -> Js.Exn.message jsexn | ||
| None -> None | ||
in | ||
let msg = Option.value msg ~default:"JSON error" in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add a comment here, explaining that Js.Exn.message can't be none in the browser (and probably true in any js environment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
The main thing we fix here is the consistent use of exceptions in native runtime, which used, before this commit either its own Of_json_error or yojson's Type_error.
Then there's
which is not required by ppx but was introduced so that the runtime modules can function as minimal JSON interfaces usable both in native and browser environments (so called universal workflow).
Before this commit the error behaviour was not specified, now we introduce
exception Of_json of string
in both native and browser and makeof_string
raise it in case of invalid JSON.Ref: #9