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

MPR#7548 Add an example of using the printf function to the manual #2272

Merged
merged 17 commits into from Mar 13, 2019
Merged

MPR#7548 Add an example of using the printf function to the manual #2272

merged 17 commits into from Mar 13, 2019

Conversation

Lereena
Copy link
Contributor

@Lereena Lereena commented Feb 27, 2019

Added a description and examples of using the printf function to the manual section "1.8 Pretty-printing", closing the MPR#7548.

@gasche
Copy link
Member

gasche commented Feb 27, 2019

Thanks for the patch proposal! The content that you propose is very nice, and it will be a good addition to the tutorial.

I think that you could have a new section to introduce Printf (for example named "Printf formats"), instead of being at the end of the "Pretty-printing" section.

The original intent of the mantis ticket you reference was to include a version of the full pretty-printer using Printf. This probably requires the use of the more advanced format specifier %a (and changing the function to take a out_channel as argument for this to work):

let print ppf expr =
  ...
  let rec print prec ppf expr =
     ...
      | Quot(f, g) ->
          open_paren prec 2 ppf;
          Printf.fprintf ppf "%a / %a" (print 2) f (print 3) g;
          close_paren prec 2 ppf

This could go at the end of the new content you wrote ("Here is how to rewrite the pretty-printer using Printf.fprintf"), probably after having introduced fprintf before --- like printf, but parametrized on the output channel. But this does not need to be part of the present Pull Request: with a separate section name, I think this is good to merge even if we don't have a port of the full pretty-printer.

(Of course the feedback of @mseri is also excellent -- but on the last point, I think the pretty-printer could wait for another PR.)

@mseri
Copy link
Member

mseri commented Feb 27, 2019

@gasche I fully agree that the pretty-printer example could be added in a second stage, and I quite like the idea of having this moved earlier on (and focusing on the easier Printf more than on Format).

Copy link
Member

@Octachron Octachron left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! I have some remarks in term of contents and emphases but this is a good start.

manual/manual/tutorials/coreexamples.etex Outdated Show resolved Hide resolved
manual/manual/tutorials/coreexamples.etex Outdated Show resolved Hide resolved
manual/manual/tutorials/coreexamples.etex Outdated Show resolved Hide resolved
manual/manual/tutorials/coreexamples.etex Show resolved Hide resolved
manual/manual/tutorials/coreexamples.etex Show resolved Hide resolved
manual/manual/tutorials/coreexamples.etex Outdated Show resolved Hide resolved
manual/manual/tutorials/coreexamples.etex Outdated Show resolved Hide resolved
@gasche
Copy link
Member

gasche commented Feb 28, 2019

@Lereena: when you have time, it would be nice if you could integrate the feedback you got from the various commenters into the PR (by adding more commits or amending your commit, and pushing to the remote branch again).

Some of the comments are a bit open-ended, and it's fine if some things are left for later, but at least easy/local change suggestions that you agree would improve the result should be handled. For the rest, you should do as you think is best to get a next iteration.

@Lereena
Copy link
Contributor Author

Lereena commented Feb 28, 2019

@gasche @mseri @Octachron Thank you very much for feedback, I will make corrections tomorrow

Copy link
Member

@mseri mseri left a comment

Choose a reason for hiding this comment

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

Nice, you also added the pretty printer example! Thanks, this is a great addition to the examples

manual/manual/tutorials/coreexamples.etex Outdated Show resolved Hide resolved
@Lereena
Copy link
Contributor Author

Lereena commented Mar 1, 2019

I corrected the wording and function calls, added links and a printer. I didn’t write about the Format module, in my opinion this is so far superfluous

Copy link
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

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

I made a couple minor comments but this looks almost ready for merging. Thanks!

manual/manual/tutorials/coreexamples.etex Outdated Show resolved Hide resolved
manual/manual/tutorials/coreexamples.etex Outdated Show resolved Hide resolved
@Lereena
Copy link
Contributor Author

Lereena commented Mar 2, 2019

@gasche I fixed wording and used an example that you proposed, thank you very much.

@Lereena
Copy link
Contributor Author

Lereena commented Mar 2, 2019

I am very sorry and extremely embarrassed that my contribution is so long and takes a lot of your attention D: Thank you for helping.

@Octachron
Copy link
Member

@Lereena , you should not feel embarrassed: it is perfectly normal to have a lot of back-and-forth between the author and editors(or reviewers) before reaching the "right" text. Clarity and simplicity sometimes requires a lot of work, and this PR is on the right track.

@Drup
Copy link
Contributor

Drup commented Mar 2, 2019

I like the idea of introducing printf in the tutorial! :)

I would suggest using pp_* as a prefix. pp is the standard name for pretty printers, and there is already a set of output_* function in Pervasive which do something quite different.

I'm also of the opinion we should advertise Format directly. Especially since in this case, the code is strictly identical. Although I understand that not everyone agrees on that.

@Lereena
Copy link
Contributor Author

Lereena commented Mar 2, 2019

I would suggest using pp_* as a prefix. pp is the standard name for pretty printers, and there is already a set of output_* function in Pervasive which do something quite different.

I agree, there can really be a misunderstanding. Made a change

Copy link
Member

@Armael Armael left a comment

Choose a reason for hiding this comment

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

A couple of minor remarks.

manual/manual/tutorials/coreexamples.etex Outdated Show resolved Hide resolved
manual/manual/tutorials/coreexamples.etex Outdated Show resolved Hide resolved
Copy link
Member

@Octachron Octachron left a comment

Choose a reason for hiding this comment

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

I have done one more round of review. The new section shaping up nicely and it should be good to go after that round.

Changes Outdated Show resolved Hide resolved
manual/manual/tutorials/coreexamples.etex Outdated Show resolved Hide resolved
manual/manual/tutorials/coreexamples.etex Outdated Show resolved Hide resolved
manual/manual/tutorials/coreexamples.etex Outdated Show resolved Hide resolved
manual/manual/tutorials/coreexamples.etex Outdated Show resolved Hide resolved
manual/manual/tutorials/coreexamples.etex Outdated Show resolved Hide resolved
manual/manual/tutorials/coreexamples.etex Outdated Show resolved Hide resolved
manual/manual/tutorials/coreexamples.etex Outdated Show resolved Hide resolved
manual/manual/tutorials/coreexamples.etex Outdated Show resolved Hide resolved
Copy link
Member

@Octachron Octachron left a comment

Choose a reason for hiding this comment

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

We are near here! I have two small remaining remarks:

manual/manual/tutorials/coreexamples.etex Show resolved Hide resolved
manual/manual/tutorials/coreexamples.etex Outdated Show resolved Hide resolved
Copy link
Member

@Octachron Octachron left a comment

Choose a reason for hiding this comment

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

I like the last version. I will merge it once CI passes. Thanks for your patience!

@Octachron Octachron merged commit f5ab75b into ocaml:trunk Mar 13, 2019
@Lereena Lereena deleted the issue0007548 branch March 13, 2019 09:53
Octachron pushed a commit that referenced this pull request Mar 16, 2019
…2272)

* Creating a new section in the first chapter of the manual

* Add an introduction of custom printer for int

* Add an easy example of %a combinator

* Add an example of using the printf function on complex data structure
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.

None yet

6 participants