I think @seliopou 's original documentation is easier to understand. Also, the paragraph you added should probably be in the interface, not the implementation.

I think I like the other change, it looks useful, but I can't really review since it's in the middle of unrelated changes about ;; and documentation. Could you split them (and also avoid the merge) ?

I know it is not that important, but could you refrain from putting back a load of unnecessary ;;s.

(I have not other thoughts on the patch for or against).

Thank you for your feed back: in fact I see that I did too much pull on my
copy of the repository: the nasty print_queue and pp_print_queue stuff came
back with the pull. I will remove them altpgether with my coment.

I will split the modifications as you suggest.

I think @seliopou 's original documentation is easier to understand. Also, the paragraph you added should probably be in the interface, not the implemented.

I think I like the other change, it looks useful, but I can't really review since it's in the middle of unrelated changes about ;; and documentation. Could you split them (and also avoid the merge) ?

---

You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
#595 (comment)

I was with @pierreweis when we looked into this patch.

@Drup We will propose a split a version in a day or two if it is ok with you. Would you suggest another PR where each commit is clearly delimited or two PRs ? I guess the latter does not make much sense ...

@lpw25 Someone else seems not to have refrained from removing or not putting ;; 😉 This is indeed the original coding style of this module (and others in the OCaml codebase). Do you really object that strongly to having a unified style at least at module level ?

@rbonichon one PR, and you don't have to make another one, just force push on this one.

Do you really object that strongly to having a unified style at least at module level ?

The unified style is not using ;;. If you want to change that style then I suggest a PR making that change across the whole standard library, rather than just making changes to one module in an unrelated pull request.

My understanding is that, at least until recently, there were two styles: with or without ;;
And they were using in different files, mostly according to the taste of the main maintainer of the file.
In particular this was the case for Format.
What was the reason to change it to start with?

@Drup Got it!

@lpw25 Could you point me to where this "unified" style is defined ? I have up until now seen no such thing in the standard library (just look at almost any module). If such a style for the compiler does not exist, it might however be a good idea to have one one day to rely on to avoid the kind of discussion we are having 😄 .

What was the reason to change it to start with?

I suppose there are two separate changes really. One is removing ;; from .mli files because it tends to break documentation comment attachment. (And to my mind is really bad style, as they are always superfluous).The other is removing them from .ml files. I did that for consistency with the rest of the standard library. I also think it is bad style, but that is less important than a consistent style.

Could you point me to where this "unified" style is defined ?

Well it doesn't really exist, but the ;; were delibrately removed by #528 and it is just a little annoying to see them being put back again in an unrelated commit.

If such a style for the compiler does not exist, it might however be a good idea to have one one day to rely on to avoid the kind of discussion we are having 😄 .

Strongly agree. (Although I only really care about the standard library for this -- because some people use it as a guideline when they are learning the language, so it can have a large influence on coding style in the wide community).

My point is mostly that such changes should not be smuggled into other PRs, otherwise we end up having these discussions on a PR which should be discussing an entirely unrelated feature/bugfix.

If you guys can write a style guide that we all agree on, we'll use it for the compiler. Good luck on getting consensus, though. In the meantime please avoid wasting your time in edit wars.

For the standard library, I accepted @lpw25's argument even though it's borderline (I don't think there's anything really wrong with teaching several styles).

Finally, I wholeheartedly agree with this:

My point is mostly that such changes should not be smuggled into other PRs, otherwise we end up having these discussions on a PR which should be discussing an entirely unrelated feature/bugfix.

@rbonichon After six months, this pull request still seems to have the main change (as per the title of this GPR) mixed up with a lot of documentation changes, some of which seem entirely unrelated. Is it possible to split those out? I think there may also be an outstanding review comment (in @Drup 's comment) about @seliopou 's documentation which I'm not sure has been addressed or refuted.

I don't think anything as changed, and if so, it's hidden in a 700 line diff of unrelated changes.

If people want review, they should make their commits easy to review. Please use add -p and make separated commits.

@mshinwell Yes this can be split. We will try to get back to it asap.
There are some related things to say:

1. Since then, regression tests build a formatter_out_functions which explains the failed checks. We will need to check that as well.
2. Regarding @seliopou's initial expose function to flush formatter's internal queue #506 request (and documentation comments), the means to achieve what he needs are provided in PR Symbolic formatted pretty-printing #615 . AFAIK, there is thus no outstanding documentation comment, though I can be mistaken.

@pierreweis and I have now split the commit into 2 separate groups as well as we could.
The second commit introduces the feature.

I think the addition is reasonable and useful. However, this is a hard breaking change for everyone using Format.set_formatter_out_functions (and you can't even modify your code to work with both the old and new version). I'll wait @damiendoligez's opinion on that, but I think that's not really acceptable.

The only way I can see to make this work reasonably is to avoid modifying the existing interface and to introduce instead a new function of type:

val set_formatting_functions: 
   ?out_string : (string -> int -> int -> unit) ->
   ?out_flush : (unit -> unit) ->
   ?out_newline : (unit -> unit) ->
   ?out_spaces : (int -> unit) ->
   ?out_indent : (int -> unit) ->
  unit -> unit

and 5 getters. This would be reasonably future proof to extend.

@Drup Yes the same problem was raise in #615 (which builds on this PR) by @alainfrisch. He offers the same solution which I think is reasonable. However, only 1 package (utop) in opam (4.03.0 compiler) uses this direct feature.

@rbonichon and next time someone wants to add a new field, are we going to break compat again ?

Might as well bite the bullet and make it future proof now.

@Drup There are two sides to this argument. On the one hand you are right (as @alainfrisch) to point out the problem of retro-compatibility. On the other hand, this record has not changed in maybe 15 years and
its direct construction is a little-used feature: breaking compat would not have a big effect on the OCaml ecosystem.

Let me repeat that your solution has its appeal and I would be fine either way. I would also accept whatever @damiendoligez thinks is best.

OK, let's break utop and its three dependencies.

As far I can tell, the two commits of this PR has been merged into trunk months ago (before the branching of 4.06) , see 98095f9 and 12642c6 . Am I missing something or should this PR be closed and a change entry added?

Yes, they were merged as part of #615. Good catch, though, for the missing Changes entry.