[Buildbot-devel] Follow up on autopep8

[Vladimir Rutsky]

On 29.10.2013 13:50, Pierre Tardy wrote:
> Friendly reminder..
>
> I'd like feedback from ewongbb and vrustky, and of course djmitche and 
> tomprince
>
> Pierre

It's hardly possible to select coding style that will be accepted by all 
Buildbot contributors.  Not following to any coding style may lead to 
more complicated development process (merge, code understanding, 
search/replace tasks), ugly and inconsistent code.

PEP8 describes well-throught-out and widely accepted in Python community 
coding style. There is tool that can check coding style (pep8), and some 
of editors and IDE already have integrated PEP8 checker.  Also there is 
automated tool for reformatting code (autopep8) that may be used (with 
proper care) for "one click" fixing code for people who doesn't care 
about "proper" spaces, comments, line endings etc.

If main Buildbot developers feels comfortable with following PEP8 (or 
part of advices in it) and/or using git hook suggested by Pierre and/or 
running autopep8, I think we should accept as much as we can from PEP8.

About fixing PEP8 issues with autopep8: I would prefer to fix issues one 
by one, but in short period of time (e.g. one or two weeks) and close to 
moment when another merge of master to nine planned.  With one by one 
fix it will be easier to see how each autopep8 issue fix looks like, 
what it breaks, what it makes better and what worse. It's impossible to 
analyze one large fix of all issues: I bet you won't be able to find at 
least one change for each fixed by autopep8 issue just by looking in 
19kloc patch.

I suggest following steps:
1) Take simple not fixed PEP8 issue, let it be W123.
2) Run autopep8 to fix it:

find master/ slave/ www/ -iname '*.py' -exec autopep8 --aggressive \
     --verbose --in-place --max-line-length=999 --select=W123 \
     '{}' \;

3) Commit and push branch on Github --- wait till Travis will end 
running tests.
4) If any tests broken: fix them and go to 3).
5) Analyze changes and discuss them with other Buildbot developers --- 
is changes ok, makes sence or poinless and makes code worse? Accept 
changes or decide to ignore W123.
6) Go to 1).

These steps can be done quite easily, trickiest things with fixing 
broken tests Pierre already done: 
https://github.com/buildbot/buildbot/pull/936

I would do this by myself, but Pierre and Edmund already started this 
work, so my commits will interfere with they work.

Not sure how hard will be merge of master into nine, suggestions about 
that was on mailing list few days ago --- I think it should be possible.

--
Vladimir Rutsky

>
>
> On Mon, Oct 28, 2013 at 3:54 PM, Dan Kegel <dank at kegel.com 
> <mailto:dank at kegel.com>> wrote:
>
>     On Mon, Oct 28, 2013 at 7:50 AM, Marc-Antoine Ruel
>     <maruel at chromium.org <mailto:maruel at chromium.org>> wrote:
>     > I agree with Pierre, enforced code formatting makes everything
>     simpler long
>     > term.
>
>     Yes.  If I recall correctly, the only thing keeping several past
>     projects
>     I worked on from adopting a strong style was the lack of an
>     autoformatter.
>     If autopep8 is as good as it sounds, having it would be a blessing.
>
>     > I also agree it's better to do it as a single commit on the
>     master branch
>     > then merge it on the nine branch, so that follow up merges are
>     simpler.
>
>     Yes (although there may need to be a residual commit on each branch
>     for the few noncommon bits).
>     - Dan
>
>
>
>
> ------------------------------------------------------------------------------
> Android is increasing in popularity, but the open development platform that
> developers love is also attractive to malware creators. Download this white
> paper to learn more about secure code signing practices that can help keep
> Android apps secure.
> http://pubads.g.doubleclick.net/gampad/clk?id=65839951&iu=/4140/ostg.clktrk
>
>
> _______________________________________________
> Buildbot-devel mailing list
> Buildbot-devel at lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/buildbot-devel

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://buildbot.net/pipermail/devel/attachments/20131030/10cc8272/attachment.html>