www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - Automatically enforce Phobos's styleguide

reply Seb <seb wilzba.ch> writes:
Hey all,

tl;dr: I want to pull improve Phobos code style for the greater 
good of having a bot that automatically checks the coding style. 
This should be quite useful for submitters. I hope we all agree 
that the time spent "nitpicking" is wasted, we overlook much (see 
below) and we can do better!

Agenda
------

The idea is quite simple:
1) Fix one violation for the entire codebase
2) Enable it in the bot.
3) Pick next violation and goto 1)

First violation: space between operators
----------------------------------------

Can be easily fixed by running this:

```
sed -i "s/switch(/switch (/" **/*.d
sed -i "s/foreach(/foreach (/" **/*.d
sed -i "s/foreach_reverse(/foreach_reverse (/" **/*.d
sed -i "s/while(/while (/" **/*.d
sed -i "s/if(/if (/" **/*.d
```

I started with some PRs (one is way to large to review):

https://github.com/dlang/phobos/pull/4239
https://github.com/dlang/phobos/pull/4240
https://github.com/dlang/phobos/pull/4241
https://github.com/dlang/phobos/pull/4242

How big is this change?
-----------------------

 grep -E "(for|foreach|foreach_reverse|if|while)\(" $(find . 
 -name '*.d') | wc -l
3505 As expected a styleguide without a automatically enforcing it, leaks ... Disadvantages ------------- This potentially might create some troubles with forks or pending PRs. -> If anyone objects the introduction of automatic linting to Phobos, please raise your voice!
Apr 26 2016
next sibling parent tsbockman <thomas.bockman gmail.com> writes:
On Tuesday, 26 April 2016 at 19:04:44 UTC, Seb wrote:
 As expected a styleguide without a automatically enforcing it, 
 leaks ...
It's not so much a matter of "leaks" - rather, much of DMD, D-runtime, and Phobos pre-date the style rules, and so never attempted to follow them in the first place. Indeed, some of the rules we have now were chosen simply by surveying the existing eclectic code base to determine which conventions were *most common* - in some cases by slim margins...
Apr 26 2016
prev sibling next sibling parent Mark Isaacson <turck11 hotmail.com> writes:
I maintain some of the linters at work. One suggestion I'd have 
(perhaps you're already on it) is that a linter that can both 
detect and prompt to auto-fix the issues is at least an order of 
magnitude more useful. People fix things more, people like the 
linters more, it avoids the fallacy of trying to get people to 
read big paragraphs of how to fix things themselves... it's just 
generally a much more awesome experience. Would recommend.
Apr 26 2016
prev sibling parent reply Seb <seb wilzba.ch> writes:
On Tuesday, 26 April 2016 at 19:04:44 UTC, Seb wrote:
 Hey all,

 tl;dr: I want to pull improve Phobos code style for the greater 
 good of having a bot that automatically checks the coding 
 style. This should be quite useful for submitters. I hope we 
 all agree that the time spent "nitpicking" is wasted, we 
 overlook much (see below) and we can do better!

 Agenda
 ------

 The idea is quite simple:
 1) Fix one violation for the entire codebase
 2) Enable it in the bot.
 3) Pick next violation and goto 1)
Great news: 1) I submitted a couple of trivial fixes (#4245, #4246, #4247) 2) The Travis bot passes :) As mentioned I decreased the linting to a minimum, but now we do have it :) Future work can fix more violations in the Phobos codebase - small low hanging fruits include e.g. > 120 lines or undocumented public methods. For more details: https://github.com/dlang/phobos/pull/4243
Apr 26 2016
parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 04/26/2016 09:36 PM, Seb wrote:
 On Tuesday, 26 April 2016 at 19:04:44 UTC, Seb wrote:
 Hey all,

 tl;dr: I want to pull improve Phobos code style for the greater good
 of having a bot that automatically checks the coding style. This
 should be quite useful for submitters. I hope we all agree that the
 time spent "nitpicking" is wasted, we overlook much (see below) and we
 can do better!

 Agenda
 ------

 The idea is quite simple:
 1) Fix one violation for the entire codebase
 2) Enable it in the bot.
 3) Pick next violation and goto 1)
Great news: 1) I submitted a couple of trivial fixes (#4245, #4246, #4247) 2) The Travis bot passes :) As mentioned I decreased the linting to a minimum, but now we do have it :) Future work can fix more violations in the Phobos codebase - small low hanging fruits include e.g. > 120 lines or undocumented public methods. For more details: https://github.com/dlang/phobos/pull/4243
Nice initiative. Thanks for doing this. Please stay on 4243 and merge it as soon as possible, so we don't have ripples following the style change from pulling other diffs. Thx! -- Andrei
Apr 27 2016
parent reply Seb <seb wilzba.ch> writes:
On Wednesday, 27 April 2016 at 12:38:55 UTC, Andrei Alexandrescu 
wrote:
 On 04/26/2016 09:36 PM, Seb wrote:
 Great news:
 1) I submitted a couple of trivial fixes (#4245, #4246, #4247)
 2) The Travis bot passes :)

 As mentioned I decreased the linting to a minimum, but now we 
 do have it :)
 Future work can fix more violations in the Phobos codebase - 
 small low
 hanging fruits include e.g. > 120 lines or undocumented public 
 methods.

 For more details:
 https://github.com/dlang/phobos/pull/4243
Nice initiative. Thanks for doing this. Please stay on 4243 and merge it as soon as possible, so we don't have ripples following the style change from pulling other diffs. Thx! -- Andrei
Just a quick update that #4243 is in and the linting bot is now running. As I mentioned earlier there are still many trivial flags of Dscanner's static analysis that could be enabled. I marked them with "FIXME", so anyone feel free to have a look. Moreover (once dub is 1.0 and released together with dmd), we want to enable the same linting in the Makefile. Mark Isaacson: Great idea, but that sounds more like a plugin for an IDE. dfmt usually fixes most of the style issues, however Dscanner does also analysis of the code.
Apr 27 2016
parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 04/27/2016 11:41 PM, Seb wrote:
 Just a quick update that #4243 is in and the linting bot is now running.
 As I mentioned earlier there are still many trivial flags of Dscanner's
 static analysis that could be enabled. I marked them with "FIXME", so
  anyone feel free to have a look.
 Moreover (once dub is 1.0 and released together with dmd), we want to
 enable the same linting in the Makefile.
Do you have an example of an error flow? -- Andrei
Apr 28 2016
parent reply Seb <seb wilzba.ch> writes:
On Thursday, 28 April 2016 at 13:46:37 UTC, Andrei Alexandrescu 
wrote:
 On 04/27/2016 11:41 PM, Seb wrote:
 Just a quick update that #4243 is in and the linting bot is 
 now running.
 As I mentioned earlier there are still many trivial flags of 
 Dscanner's
 static analysis that could be enabled. I marked them with 
 "FIXME", so
  anyone feel free to have a look.
 Moreover (once dub is 1.0 and released together with dmd), we 
 want to
 enable the same linting in the Makefile.
Do you have an example of an error flow? -- Andrei
I accidentally proved that it does check for errors: https://github.com/dlang/phobos/pull/4255 You can see the history here: https://travis-ci.org/dlang/phobos/builds
Apr 28 2016
parent Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 04/28/2016 09:50 AM, Seb wrote:
 On Thursday, 28 April 2016 at 13:46:37 UTC, Andrei Alexandrescu wrote:
 On 04/27/2016 11:41 PM, Seb wrote:
 Just a quick update that #4243 is in and the linting bot is now running.
 As I mentioned earlier there are still many trivial flags of Dscanner's
 static analysis that could be enabled. I marked them with "FIXME", so
  anyone feel free to have a look.
 Moreover (once dub is 1.0 and released together with dmd), we want to
 enable the same linting in the Makefile.
Do you have an example of an error flow? -- Andrei
I accidentally proved that it does check for errors: https://github.com/dlang/phobos/pull/4255 You can see the history here: https://travis-ci.org/dlang/phobos/builds
Hmmm... not to clear error messages - something we could improve in the near future. -- Andrei
Apr 28 2016