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
custom format #6452
Comments
Comment author: @diml Added a patch to make custom functions always return a string. To avoid pinning the format to one specific class of printf functions. |
Comment author: @diml Merged in 4.02 (revisions 16124 and 16125). |
Comment author: @gasche Thanks for taking care of the 4.02 merge. |
Comment author: bvaugon I submitted a new version that complete the custom format In this version, instead of computing a string, a custom node takes a In addition, I suggest that a custom node now takes a fourth argument, The printers and scanners may be arbitrary functions that manually {[ let printer mode fmt n b = match mode with let scanner ib f = Scanf.bscanf ib "[%X/%B]" f let fmt = Custom (arity, printer, scanner, End_of_format) The first argument, "arity", is an integer in a à la Peano The second argument is the printer, it is called with a mode, a The scanning function takes as arguments a Scanf.Scanning.in_channel The last argument is the rest of the format. As the Jeremie one, this implementation is of course type safe (no This patch however has some drawbacks :
|
Comment author: @gasche (As we already discussed together,) I am unconvinced by the idea of adding a Format dependency to the format-string logic and implementation. I think that Printf and Scanf are reasonably general in their interface, but there are many other pretty-printing libraries than Format in the OCaml world that make different assumptions. (Of course the dependency is already there to some degree, in the format of the dedicated formatting converters in format strings (eg. "[<h 3>...]"), but I think a hard dependency on the internal type of Format buffers is qualitatively different.) (One possibility would be to remove the Format support of this patch, but keep a mode argument with only one constructor calling_from_Printf, keeping the door open for other modes in the future.) Jérémie, would you be interested in either the Format or the Scanf support? I'm reluctant to consider a change as invasive as the proposed patch just weeks before a minor release. Is there a strong consensus among the people that care about format strings? |
Comment author: bvaugon Yes, but remarks that having the support of Format features in Custom Moreover, there is no new dependencies to the Format module nor But you are right, this patch is huge (not really because it consits |
Comment author: @gasche I think that there is something fundamentally problematic with importing a complete pretty-printing library logic into a module that currently only handle formatting strings. There are some weird things with the patch as proposed, for example:
|
Comment author: bvaugon The reason is that the Scanning.in_channel has been moved in The old "in_channel_name" (declaring the constructor From_file) made A Buffer.t was also mentionned in the type Scanning.in_channel and for Since these modifications only concern internals of the stdlib (I |
Comment author: @diml
If Format was supported, for the !"%{sexp:type}" syntax custom_printf would use [Sexp.pp] when called from Format. Same for Scanf, !"{sexp:type}" could allow to parse a S-expression. But I don't think there is a high demand for it, at Jane Street for instance we use custom_printf a lot more than Format or Scanf. |
Comment author: @gasche I'm thinking of not trying to rush this patch into 4.02, but eventually merge the feature in 4.03. That would mean that the printf-relevant interface would be different between the two major versions. Is that ok for you? |
Comment author: @diml That's OK for me. |
Comment author: @gasche Just a note to whoever may wonder what happen{s,ed} to this patch. I am still interesting in merging improved Custom_format in trunk, but I am still a bit worried by the bulkiness of Benoît's patch and in particular the import of a large quantity of pretty-printing logic deeper in the OCaml distribution's entrails. I would be interested in knowing what the Scanf-relevant part of this patch looks like, but I personally lack the time to play with the current patch in this way. If someone wished to move this forward, I think a second opinion on "can this proposal be made less invasive?", or maybe even "is there a better compromise in term of expressiveness / invasiveness ratio?" would be helpful. |
Comment author: @Drup I don't have much opinion on the presumed "bulkiness" of the patch (except that moving code around with barely changing it, in a backward compatible manner, is not my definition of "invasive"), however I do have an opinion on the feature. As I mentioned in ppx_custom_printf's bug tracker1, composing with Maybe janestreet doesn't care about proper indentation of pretty printed output, but I do, and as much as I'm interested by ppx_custom_printf, It's unusable for me at the moment due to this limitation. |
Comment author: @diml Note: since the patch moves a lot of code around, it'd be nice to provide diffing commands, such as: patdiff <(git show :stdlib/format.ml | sed -n 12,42p) stdlib/camlinternalFormat.mli It will make it easier for people to review the changes (once the patch has been rebased to the current trunk and submitted as a pull request on github). |
Comment author: @gasche I'm moving this to 4.04; I think we are too late in the 4.03 cycle to make a decision on the different implementation. This means that, if we integrate the new interface in a future release, existing code using Custom_format will have to be changed. As long as everyone understands that the internal format definitions are still subject to future change, this is fine. I would be happy if someone else wished to take decisions on this work. (I was not assigned on the issue, but consider that I am explicit un-assigning myself anyway.) |
Comment author: @diml OK, I assigned the issue to myself. I won't have time to look at this in the near future, but I'll try to come back to this later |
This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc. |
This was added in #140 |
Original bug ID: 6452
Reporter: @diml
Assigned to: @diml
Status: assigned (set by @diml on 2016-04-19T12:52:03Z)
Resolution: reopened
Priority: normal
Severity: feature
Target version: undecided
Category: standard library
Monitored by: @Drup bvaugon @hcarty @yakobowski
Bug description
It would be good if we could put custom functions into a format. There doesn't need to be a syntax for it, this could be the job of a ppx.
I'm thinking of something like this:
| Custom :
('b -> 'x -> 'c) * ('a, 'b, 'c, 'd, 'e, 'f) fmt ->
('x -> 'a, 'b, 'c, 'd, 'e, 'f) fmt
This would allow to write things like github.com/janestreet/custom_printf in a much cleaner way. At the moment we modify the call site to stuff the conversion functions in the list of arguments: it's not pretty and becoming more and more complex. With the suggested modification we could simply 'map' the format and stuff the conversion functions in it.
I don't have a use case but it might also be interesting to allow custom functions to take 0 or more than 1 argument, here is a more generic proposal:
| Custom :
('x, 'y, 'a, 'c) custom_arity * ('b -> 'y) * ('a, 'b, 'c, 'd, 'e, 'f) fmt ->
('x, 'b, 'c, 'd, 'e, 'f) fmt
type (_, _, _, _) custom_arity =
| Zero : ('a, 'b, 'a, 'b) custom
| Succ : ('a, 'b, 'c, 'd) custom -> ('x -> 'a, 'x -> 'b, 'c, 'd) custom
File attachments
The text was updated successfully, but these errors were encountered: