Expand documentation of exceptions #148
Conversation
sim642
requested review from
NathanReb,
panglesd and
Leonidas-from-XIV
as code owners
There was a problem hiding this comment.
These are some very nice improvements. Thanks for your work investigating and documenting the exceptions - it is rather thankless to figure it out and having an exception raised without being aware of it is definitely a source of frustration.
I pointed out a few notes on how to improve the PR, mostly for consistency/clarity.
lib/common.mli
Outdated
| @@ -1,8 +1,10 @@ | |||
| val version : string | |||
|
|
|||
| exception Json_error of string | |||
| (** Exception (usually) describing a parsing failure. *) | |||
There was a problem hiding this comment.
I think the "usually" should be qualified a bit better because I think now it is adding more questions than answering those.
There was a problem hiding this comment.
I just expanded the description to list both usages.
lib/write.ml
Outdated
| @@ -121,7 +121,7 @@ let float_needs_period s = | |||
|
|
|||
| The _fast version is faster but often produces unnecessarily long numbers. | |||
| *) | |||
| let write_float_fast ob x = | |||
| let write_float_fast ob x = (* unused *) | |||
There was a problem hiding this comment.
Can't really say that it is unused, since this is in the .mli, so it might be used by some code on OPAM.
Though it might very well be that nobody is using it, in which case we should consider removing it.
There was a problem hiding this comment.
At least according to sherlocode, nothing on opam uses this. Since it's hidden from the documentation as well, I suppose there doesn't have to be any stability guarantees for this part of the interface. Maybe something at some point needed this (also atdgen?).
If you'd like, I can remove this comment from this PR, but the actual removal of the function is best to be left as a separate matter.
There was a problem hiding this comment.
Thanks for the research. I agree removing this would not make sense in the scope of this PR, but if you happen to have a moment after this is merged to remove the unused functions that would be nice. Otherwise I'll probably get to it… eventually.
Of course, there might be some non-public code that uses these functions, but accumulating tons of functions without a clear usecase is somewhat of an anti-pattern and makes maintenance harder.
There was a problem hiding this comment.
I created the draft PR #149 for this.
There was a problem hiding this comment.
Very nice! Can you please add a note to the changelog about your changes (short note, PR number and your GH handle)? The semantics of the code don't change but users get better documentation so I would classify that as user-facing change and worth mentioning.
Done! |
|
Excellent, thanks for the work! |
CHANGES: *2022-08-09* ### Added - Expanded documentation of exceptions (@sim642, ocaml-community/yojson#148) ### Removed - Removed undocumented and unused functions `write_float_fast` and `write_std_float_fast` from `Yojson`, `Yojson.Basic` and `Yojson.Safe` (@sim642, ocaml-community/yojson#149) ### Fixed - Fix out-of-bounds error occurring when parsing object field names with atdgen parsers using `map_ident` or `map_lexeme` (@mjambon, ocaml-community/yojson#150)
Primarily makes it explicit that parsing errors raise
Json_error, which isn't on the same page as parsing functions and didn't have any description. Surprisingly, serialization may also raise it in a corner case.Secondarily, I reviewed all other raised exceptions to consistently document those as well (using
@raise).