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

Adding a new field to record `formatter_out_functions` to redefine the meaning of indentation #595

Closed
wants to merge 2 commits into
base: trunk
from

Conversation

Projects
None yet
10 participants
@pierreweis
Contributor

pierreweis commented May 23, 2016

All output of the pretty-printer were abstracted in record formatter_out_functions except for indentation which was linked to output spaces. Indentation could be more complex than simply adding spaces at begining of lines.
The expected impact to existing software should be minimal.

Documentation has been update and carefully checked.

@Drup

This comment has been minimized.

Contributor

Drup commented May 23, 2016

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) ?

@lpw25

This comment has been minimized.

Contributor

lpw25 commented May 23, 2016

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).

@pierreweis

This comment has been minimized.

Contributor

pierreweis commented May 23, 2016

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)

@rbonichon

This comment has been minimized.

Contributor

rbonichon commented May 24, 2016

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 ?

@Drup

This comment has been minimized.

Contributor

Drup commented May 24, 2016

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

@lpw25

This comment has been minimized.

Contributor

lpw25 commented May 24, 2016

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.

@garrigue

This comment has been minimized.

Contributor

garrigue commented May 24, 2016

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?

@rbonichon

This comment has been minimized.

Contributor

rbonichon commented May 24, 2016

@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 😄 .

@lpw25

This comment has been minimized.

Contributor

lpw25 commented May 24, 2016

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.

@damiendoligez

This comment has been minimized.

Member

damiendoligez commented May 31, 2016

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.

@mshinwell

This comment has been minimized.

Contributor

mshinwell commented Dec 28, 2016

@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.

@Drup

This comment has been minimized.

Contributor

Drup commented Dec 28, 2016

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.

@rbonichon

This comment has been minimized.

Contributor

rbonichon commented Dec 29, 2016

@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 #506 request (and documentation comments), the means to achieve what he needs are provided in PR #615 . AFAIK, there is thus no outstanding documentation comment, though I can be mistaken.
@rbonichon

This comment has been minimized.

Contributor

rbonichon commented Feb 7, 2017

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

@Drup

This comment has been minimized.

Contributor

Drup commented Feb 7, 2017

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.

@rbonichon

This comment has been minimized.

Contributor

rbonichon commented Feb 7, 2017

@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.

@Drup

This comment has been minimized.

Contributor

Drup commented Feb 7, 2017

@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.

@rbonichon

This comment has been minimized.

Contributor

rbonichon commented Feb 7, 2017

@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.

@damiendoligez

This comment has been minimized.

Member

damiendoligez commented Mar 7, 2017

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

rbonichon added some commits Feb 6, 2017

[format] Documentation fixes and enhancements
- Typos
- English
- Extended documentation for semantic tags
[format] Introduce indentation as a separate output feature
Before this commit, indentation was identical to outputting blanks.
This is generally true but not always: in some applications you may want to use
'\t' as indentation but not for blanks, ...

The `formatter_out_functions` record is extended with an extra settable field
`out_indent`.

This allows the user to control this aspect of pretty-printing.

The teststuite has been update to reflect this modification.

@damiendoligez damiendoligez added this to the 4.07-or-later milestone Sep 27, 2017

@Octachron

This comment has been minimized.

Contributor

Octachron commented Oct 21, 2017

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?

@gasche

This comment has been minimized.

Member

gasche commented Oct 21, 2017

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

@gasche gasche closed this Oct 21, 2017

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