On 01/12/2016 08:42 AM, Martin Drašar via Digitalmars-d wrote:
> Wouldn't it be sufficient to mandate usage of dfmt with proper settings
> before submitting a PR?

That would suffice at least in the beginning. We also need to put dfmt in tools, again a project that's been in limbo for a long time. -- Andrei

On Tuesday, 12 January 2016 at 13:34:25 UTC, Andrei Alexandrescu wrote:
> Related to https://github.com/D-Programming-Language/dlang.org/pull/1191:
>
> A friend who is in the GNU community told me a while ago they have a mechanical style checker that people can run against their proposed patches to make sure the patches have a style consistent with the one enforced by the GNU style. I forgot the name, something like "good-patch". I'll shoot him an email.
>
> Similarly, I think it would help us to release a tool in the tools/ repo that analyzes a would-be Phobos pull request and ensures it's styled the same way as most of Phobos: braces on their own lines, whitespace inserted a specific way, no hard tabs, etc. etc. Then people can run the tool before even submitting a PR to make sure there's no problem of that kind.
>
> Over the years I've developed some adaptability to style, and I can do Phobos' style no problem even though it wouldn't be my first preference (I favor Egyptian braces). But seeing a patchwork of styles within the same project, same file, and sometimes even the same pull request is quite jarring at least to me.
>
> Who would like to embark on writing such a tool?
>
>
> Thanks,
>
> Andrei

There is already an attempt to it in DMD: https://github.com/D-Programming-Language/dmd/blob/master/src/checkwhitespace.d
Agree it would be useful to have a tool which needs to be part of the auto-tester (I think it's called from the Makefile for DMD).

We could also provide a `post-commit-hook` that people can use so git checks it automatically. It needs to be checked on a per-commit basis because else we'll end up with commits affecting each other, which is no good either.

On 2016-01-12 15:53, Adam D. Ruppe wrote:

> I'm not sure if git supports this but I think it should be done fully
> automatically. Not even something the user runs, just when they open the
> pull request, it reformats the code.

The hook/tool would need to do a commit with the changes. How would what work? The tool wouldn't have commit access to the repository from where the PR originates. It would also create a new commit hash that wouldn't match with what the user have locally.

-- 
/Jacob Carlborg

On Tuesday, 12 January 2016 at 21:04:33 UTC, Jacob Carlborg wrote:
> On 2016-01-12 15:53, Adam D. Ruppe wrote:
>
>> I'm not sure if git supports this but I think it should be done fully
>> automatically. Not even something the user runs, just when they open the
>> pull request, it reformats the code.
>
> The hook/tool would need to do a commit with the changes. How would what work? The tool wouldn't have commit access to the repository from where the PR originates. It would also create a new commit hash that wouldn't match with what the user have locally.

Yeah, don't know how that can be made to work.

The closest I got was https://help.github.com/articles/about-webhooks/ at least then you can check whether dfmt agrees with the pull request (and block it)

But people still need to manually run the thing.

On Tue, Jan 12, 2016 at 02:03:57PM -0500, Andrei Alexandrescu via Digitalmars-d wrote:
> On 01/12/2016 08:42 AM, Martin Drašar via Digitalmars-d wrote:
> >Wouldn't it be sufficient to mandate usage of dfmt with proper settings before submitting a PR?
> 
> That would suffice at least in the beginning. We also need to put dfmt in tools, again a project that's been in limbo for a long time. -- Andrei

In principle, I agree with mechanical checking of formatting (instead of the endless tedious nitpicking over the fine points of Phobos style), but, as Jonathan has brought up before, there are cases where human judgment is required and a tool would probably make a big mess of things.

For example, certain unittests in std.datetime where customized formatting of array literals are used to make it easier to read the test cases, such as `testGregDaysBC` in std/datetime.d, as well as the several array literals inside the same unittest block. This is just one of many examples one can find in std.datetime.

There are also some (smaller) examples in std.range, such as in
transposed(), where nested arrays are formatted like matrices in order
to make it clear what the function is trying to do. I'm almost certain
dfmt (or any mechanical formatting tool) will completely ruin this.

These formatting exceptions are primarily semantically-motivated; as
such I don't expect a mechanical tool to be able to figure it out
automatically. (E.g., nested arrays in transposed() may need 2D
formatting, but in byChunk() it makes more sense to format them linearly
with line-wrapping.)

I propose that a better approach is to automate dfmt (or whatever tool) to check PRs and highlight places where the formatting deviates from the mechanical interpretation of the rules, so that submitters can have their attention brought to potentially problematic points, and reviewers don't have to scrutinize every line, but only look at the places that may require some human judgment to decide on.


T

-- 
"640K ought to be enough" -- Bill G. (allegedly), 1984. "The Internet is not a primary goal for PC usage" -- Bill G., 1995. "Linux has no impact on Microsoft's strategy" -- Bill G., 1999.

On Wednesday, 13 January 2016 at 05:19:36 UTC, H. S. Teoh wrote:
> There are also some (smaller) examples in std.range, such as in
> transposed(), where nested arrays are formatted like matrices in order
> to make it clear what the function is trying to do. I'm almost certain
> dfmt (or any mechanical formatting tool) will completely ruin this.

It will. There's a solution in the form of special comments:

// dfmt off
auto treeStructure = [
    node(0, 0),
        node(1, 0),
        node(2, 0),
            node(10, 2),
        node(3, 0)
];
// dfmt on

// dfmt off
stuff.map!(a => complicatedFunction(a, 100) * 20)
     .filter!(a => a < 2_000 && a %3 == 0)
     .sum();
// dfmt on

> These formatting exceptions are primarily semantically-motivated; as
> such I don't expect a mechanical tool to be able to figure it out
> automatically. (E.g., nested arrays in transposed() may need 2D
> formatting, but in byChunk() it makes more sense to format them linearly
> with line-wrapping.)

Correct.

> I propose that a better approach is to automate dfmt (or whatever tool) to check PRs and highlight places where the formatting deviates from the mechanical interpretation of the rules, so that submitters can have their attention brought to potentially problematic points, and reviewers don't have to scrutinize every line, but only look at the places that may require some human judgment to decide on.

This might work.

On 1/13/16 12:19 AM, H. S. Teoh via Digitalmars-d wrote:
> On Tue, Jan 12, 2016 at 02:03:57PM -0500, Andrei Alexandrescu via Digitalmars-d wrote:
>> On 01/12/2016 08:42 AM, Martin Drašar via Digitalmars-d wrote:
>>> Wouldn't it be sufficient to mandate usage of dfmt with proper
>>> settings before submitting a PR?
>>
>> That would suffice at least in the beginning. We also need to put dfmt
>> in tools, again a project that's been in limbo for a long time. --
>> Andrei
>
> In principle, I agree with mechanical checking of formatting (instead of
> the endless tedious nitpicking over the fine points of Phobos style),
> but, as Jonathan has brought up before, there are cases where human
> judgment is required and a tool would probably make a big mess of
> things.

BTW I recall it was your code containing a "switch(x){" that prompted this idea :o). True story. -- Andrei