Add a few utility functions in Misc#2284
Conversation
trefis
left a comment
There was a problem hiding this comment.
The Option module exists in the stdlib. Shouldn't you try using it, instead of Misc.Stdlib.Option?
Also, the one in Misc I believe duplicates some of the functions of the one in the stdlib. Perhaps it would be better to look into cleaning Misc.Stdlib instead of growing it even more.
| loop 0 | ||
|
|
||
| let print ppf t = | ||
| Format.pp_print_string ppf t |
|
Indeed, I think these probably predate the support in the |
a9849d4 to
ded3098
Compare
|
@trefis Please review again. I have removed the functions from |
trefis
left a comment
There was a problem hiding this comment.
The cleanup of Misc.Stdlib.Option is nice!
Adding a printer in there seems reasonable.
The addition of Misc.protect_writing_to_file is also fine.
I would remove the alias to Format.pp_print_string from Misc.Stdlib.String though.
|
Also, I tend to agree with Alain: I'm not sure there is much value in a changelog entry for that PR. |
|
The |
I agree with that on principle, the issue is dependency order of stdlib modules. At some point, we need to have a serious though about the exact dependency orders we want for the stdlib modules. I remember @dbuenzli being pretty burned by that for the That being said, the convention for pretty printing functions is |
|
@Drup Well, we need to have a serious discussion as to how printing can be added to the data structure modules in the stdlib. Let's do that elsewhere though... |
Just for the ref this was https://caml.inria.fr/mantis/view.php?id=7500 |
We're not talking about the stdlib here, only of Since people think it's fine I'll retract my comment and approve. |
trefis
left a comment
There was a problem hiding this comment.
Feel free to merge once you've rebased.
ded3098 to
4b60703
Compare
This GPR provides a few straightforward functions in
Miscthat are required by the forthcoming work on symbol types. I don't think there is value in breaking this patch down into smaller pieces, given current time constraints.