-
Notifications
You must be signed in to change notification settings - Fork 60
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
Expand documentation of exceptions #148
Changes from 3 commits
aff918c
373a683
e39d3a1
a08f745
6aaecec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't really say that it is unused, since this is in the 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I created the draft PR #149 for this. |
||
match classify_float x with | ||
FP_nan -> | ||
Buffer.add_string ob "NaN" | ||
|
@@ -175,7 +175,7 @@ let write_normal_float_prec significant_figures ob x = | |
if float_needs_period s then | ||
Buffer.add_string ob ".0" | ||
|
||
let write_float_prec significant_figures ob x = | ||
let write_float_prec significant_figures ob x = (* used by atdgen *) | ||
sim642 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
match classify_float x with | ||
FP_nan -> | ||
Buffer.add_string ob "NaN" | ||
|
@@ -190,7 +190,7 @@ let json_string_of_float x = | |
Buffer.contents ob | ||
|
||
|
||
let write_std_float_fast ob x = | ||
let write_std_float_fast ob x = (* unused *) | ||
match classify_float x with | ||
FP_nan -> | ||
json_error "NaN value not allowed in standard JSON" | ||
|
@@ -226,7 +226,7 @@ let write_std_float ob x = | |
if float_needs_period s then | ||
Buffer.add_string ob ".0" | ||
|
||
let write_std_float_prec significant_figures ob x = | ||
let write_std_float_prec significant_figures ob x = (* used by atdgen *) | ||
match classify_float x with | ||
FP_nan -> | ||
json_error "NaN value not allowed in standard 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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just expanded the description to list both usages.