Skip to content

Longident.to string#1918

Closed
gasche wants to merge 3 commits intoocaml:trunkfrom
gasche:Longident.to_string
Closed

Longident.to string#1918
gasche wants to merge 3 commits intoocaml:trunkfrom
gasche:Longident.to_string

Conversation

@gasche
Copy link
Copy Markdown
Member

@gasche gasche commented Jul 19, 2018

Longident.to_string : t -> string is something which I would have needed while implementing ocaml-ppx/ppx_import#25

I thought that I was writing the tests out of principle, but in fact my first version was buggy, it would print (F)X instead of F(X). (One of the worst notations for applications.)

@Octachron
Copy link
Copy Markdown
Member

There is Printtyp.longident: Format.formatter -> Longident.t -> unit which is quite similar. Should the two functions be grouped together?

@Drup
Copy link
Copy Markdown
Contributor

Drup commented Jul 19, 2018

Damn, @Octachron was faster. In addition, please write a pp function first, and then to_string in term of it if you really want it.

@gasche
Copy link
Copy Markdown
Member Author

gasche commented Jul 19, 2018

@Octachron re. MPR#7827, my new attempt with expect tests does not work very well:

     ... testing 'expect_test_longident.ml' with 1 (expect) => >> Fatal error: Longident.flat
    >> Fatal error: Longident.flat

@gasche gasche force-pushed the Longident.to_string branch from c91bfff to 06ba0fe Compare July 19, 2018 21:07
@gasche
Copy link
Copy Markdown
Member Author

gasche commented Jul 19, 2018

I implemented the suggestion to move Printtyp.longident into Longident.pp in 06ba0fe.

@Octachron
Copy link
Copy Markdown
Member

I think that this is more a redirection issue: expect_test only redirect formatters: since Misc fatal errors directly print on stderr they are not captured.

@gasche gasche force-pushed the Longident.to_string branch from 06ba0fe to f8e0594 Compare July 19, 2018 21:11
@gasche
Copy link
Copy Markdown
Member Author

gasche commented Jul 19, 2018

@Octachron do you foresee any issue with changing Misc.fatal_error to use Format.eprintf instead? Misc.fatal_errorf is already using it.

@Octachron
Copy link
Copy Markdown
Member

I do not foresee any problem (and removing one layer in fatal_errorf sounds like a good idea).

gasche added a commit to gasche/ocaml that referenced this pull request Jul 19, 2018
The use of a non-Format output caused troubles when called from the
toplevel. In comparison, Misc.fatal_errorf was already using Format,
and many files freely mix those two functions.

The suggestion comes from Florian Angeletti in ocaml#1918.

Reviewed by: ?

(no change entry needed)
@shindere
Copy link
Copy Markdown
Contributor

shindere commented Jul 19, 2018 via email

@Octachron
Copy link
Copy Markdown
Member

@shindere , you can convert from a pp function to a to_string function with

let to_string_from_printer pp = Format.asprintf "%a" pp ;;
(* which is Fmt.to_to_string *)

Internally, Format will use a buffer and extract the printed string at the end.

@shindere
Copy link
Copy Markdown
Contributor

shindere commented Jul 20, 2018 via email

@Octachron
Copy link
Copy Markdown
Member

Format.asprintf generates a new formatter from a fresh buffer before printing to it, thus the result does not depend on the former content of any formatter.

@shindere
Copy link
Copy Markdown
Contributor

shindere commented Jul 21, 2018 via email

in String.concat "" (print lid [])

let rec pp ppf = function
| Lident s -> Format.pp_print_string ppf s
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does Lident "~+" need special treatment?


let rec pp ppf = function
| Lident s -> Format.pp_print_string ppf s
| Ldot(p, s) -> Format.fprintf ppf "%a.%s" pp p s
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about cases like L.( = ) ?

@gasche
Copy link
Copy Markdown
Member Author

gasche commented Jul 23, 2018

@jberdine excellent questions, thanks! I'll have to look at pprintast to check how that is done there. (The fact that Printtyp.longident uses the exact same printing logic gives me some confident that the naive/direct approach is useful for most use-cases, but it's nice to be more complete.)

@Octachron
Copy link
Copy Markdown
Member

Octachron commented Jul 23, 2018

For types, there is no special identifiers, so I don't think that Printtyp.longident considered the problem.

@gasche
Copy link
Copy Markdown
Member Author

gasche commented Aug 20, 2018

Superseded by #1996.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants