Skip to content
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

Use Format for pretty-printing instead of Fmt #140

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Use Format for pretty-printing instead of Fmt #140

wants to merge 1 commit into from

Conversation

keleshev
Copy link
Contributor

@keleshev keleshev commented Feb 9, 2019

This is probably a bit random, but I replaced Fmt with the standard Format module. One less dependency, plus if you like printf you might find this more readable.

I hope to get onto something more useful (like block strings) soon

@andreas
Copy link
Owner

andreas commented Feb 10, 2019

Looks good, thanks! 👍 I'm happy to merge this when 0.9.0 is merged in OPAM -- just in case there are any last minute changes required before then.

@hannesm
Copy link
Contributor

hannesm commented Feb 10, 2019

what is the advantage hereof? I honestly find the Fmt combinators much easier to read than Format (also, the diff +100 -84 shows that there is actually more code involved).

@keleshev
Copy link
Contributor Author

@hannesm the advantage is dubious, honestly. Readability is subjective. I find the Format style more readable, but whatever the maintainers find most suited to their needs.

The +100 -84 diff is mostly due to more liberal line breaks:

(* Before *)
Fmt.fmt "%a %s%a%a %a" fmt pp_optype op.optype name variables op.variable_definitions directives op.directives selection_set op.selection_set

(* After *)
fprintf ppf "%s %s%a%a %a"
  (optype_to_string op.optype)
  name
  pp_variables op.variable_definitions
  pp_directives op.directives
  pp_selection_set op.selection_set

I find that Format is adequate, except for hideous names, which you can fix by doing "import as":

let list = Format.pp_print_list

@andreas
Copy link
Owner

andreas commented Feb 11, 2019

I don't mind much either way. I hope this can be motivating for further contributions though, so I'm happy to indulge 😄

@hongchangwu hongchangwu mentioned this pull request Sep 15, 2019
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.

3 participants