www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - "Good PR" mechanical check

reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
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
Jan 12 2016
next sibling parent reply =?UTF-8?Q?Martin_Dra=c5=a1ar?= via Digitalmars-d writes:
Dne 12.1.2016 v 14:34 Andrei Alexandrescu via Digitalmars-d napsal(a):

 Related to https://github.com/D-Programming-Language/dlang.org/pull/119=


1:

=20
 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.
=20
 Similarly, I think it would help us to release a tool in the tools/ rep=


o

 that analyzes a would-be Phobos pull request and ensures it's styled th=


e

 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 th=


e

 tool before even submitting a PR to make sure there's no problem of tha=


t

 kind.
=20
 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.
=20
 Who would like to embark on writing such a tool?
=20
=20
 Thanks,
=20
 Andrei


Wouldn't it be sufficient to mandate usage of dfmt with proper settings
before submitting a PR?

Martin
Jan 12 2016
parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
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
Jan 12 2016
parent reply "H. S. Teoh via Digitalmars-d" <digitalmars-d puremagic.com> writes:
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.
Jan 12 2016
next sibling parent reply Brian Schott <briancschott gmail.com> writes:
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.
Jan 12 2016
parent "H. S. Teoh via Digitalmars-d" <digitalmars-d puremagic.com> writes:
On Wed, Jan 13, 2016 at 07:53:45AM +0000, Brian Schott via Digitalmars-d wrote:

 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


[...]

Fair enough, but will this work in ddoc'd unittests, where you don't
necessarily want those dfmt comments to make it into the generated docs?

But, case in point, won't UFCS chains be poorly formatted in general? Or
are there dfmt comments that can switch dfmt to "UFCS mode"? Or does it
detect UFCS chains and format accordingly?


T

-- 
To err is human; to forgive is not our policy. -- Samuel Adler
Jan 13 2016
prev sibling parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
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
Jan 13 2016
parent "H. S. Teoh via Digitalmars-d" <digitalmars-d puremagic.com> writes:
On Wed, Jan 13, 2016 at 08:47:19AM -0500, Andrei Alexandrescu via Digitalmars-d
wrote:

 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


Haha, was it?  I don't recall anymore. :-P


T

-- 
Не дорог подарок, дорога любовь.
Jan 13 2016
prev sibling next sibling parent Rikki Cattermole <alphaglosined gmail.com> writes:
On 13/01/16 2:34 AM, 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


dfmt + create patch compared to before and after format.
No need for a whole new tool.
Jan 12 2016
prev sibling next sibling parent reply Adam D. Ruppe <destructionator gmail.com> writes:
On Tuesday, 12 January 2016 at 13:34:25 UTC, Andrei Alexandrescu 
wrote:

 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



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.

I'm pretty sure dfmt is up to the task in 99% of cases already.
Jan 12 2016
next sibling parent w0rp <devw0rp gmail.com> writes:
I think using dfmt for this is a good idea. If there any problems 
with dfmt which would prevent it from being used on Phobos, the 
problems can be patched and then that would strengthen dfmt.
Jan 12 2016
prev sibling next sibling parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 1/12/2016 6:53 AM, Adam D. Ruppe wrote:

 I'm pretty sure dfmt is up to the task in 99% of cases already.


The last 1% always takes 99% of the dev time :-(
Jan 12 2016
parent John Colvin <john.loughran.colvin gmail.com> writes:
On Tuesday, 12 January 2016 at 17:22:16 UTC, Walter Bright wrote:

 On 1/12/2016 6:53 AM, Adam D. Ruppe wrote:

 I'm pretty sure dfmt is up to the task in 99% of cases already.


 The last 1% always takes 99% of the dev time :-(


But in this case, the 1% doesn't actually have to be fixed 
(although of course, the smaller the better), it's just the 1% of 
the work left to be done manually, where currently we do 100% 
manually.
Jan 12 2016
prev sibling parent reply Jacob Carlborg <doob me.com> writes:
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
Jan 12 2016
parent Sebastiaan Koppe <mail skoppe.eu> writes:
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.
Jan 12 2016
prev sibling next sibling parent reply tsbockman <thomas.bockman gmail.com> writes:
On Tuesday, 12 January 2016 at 13:34:25 UTC, Andrei Alexandrescu 
wrote:

 [...]


I realize that dfmt may need some upgrades first, but isn't it 
about time to just suck it up and dfmt the whole of phobos and 
druntime?

It will mess with the "git blame", true - but it will do so 
*once* and end tedious manual inspection of formatting for pull 
requests forever.
Jan 12 2016
next sibling parent rsw0x <anonymous anonymous.com> writes:
On Tuesday, 12 January 2016 at 18:25:48 UTC, tsbockman wrote:

 On Tuesday, 12 January 2016 at 13:34:25 UTC, Andrei 
 Alexandrescu wrote:

 [...]


 I realize that dfmt may need some upgrades first, but isn't it 
 about time to just suck it up and dfmt the whole of phobos and 
 druntime?

 It will mess with the "git blame", true - but it will do so 
 *once* and end tedious manual inspection of formatting for pull 
 requests forever.


Doubt I'm alone in that I'd be more willing to contribute if 
phobos and druntime used dfmt. I personally use a style near the 
complete opposite of phobos/druntime (2 spaces per indent, OTBS 
etc.) It's a chore to have to scrutinize every line to make sure 
it matches the style guide instead of just running it through a 
tool.
Jan 12 2016
prev sibling parent Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 01/12/2016 01:25 PM, tsbockman wrote:

 On Tuesday, 12 January 2016 at 13:34:25 UTC, Andrei Alexandrescu wrote:

 [...]


 I realize that dfmt may need some upgrades first, but isn't it about
 time to just suck it up and dfmt the whole of phobos and druntime?

 It will mess with the "git blame", true - but it will do so *once* and
 end tedious manual inspection of formatting for pull requests forever.


I would support that. We need a strong champion there. Brian wanted to 
get to it for a long time, but didn't muster the time. -- Andrei
Jan 12 2016
prev sibling parent Mathias Lang <pro.mathias.lang gmail.com> writes:
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.
Jan 12 2016