Coding Style

[Tim Flink]

On Fri, 12 Jun 2015 09:21:38 -0400 (EDT)
Kamil Paral <kparal at redhat.com> wrote:

> > > Back to everyone's favorite topic - coding style and enforcement
> > > of that style.
> 
> And I thought we were doing so great by leaving it behind...
> 
> > > 
> > > I'm not picking on Josef here - I'm sure I've submitted code
> > > recently with lint errors, this was just the review I was looking
> > > at which triggered the idea:
> > > 
> > > https://phab.qadevel.cloud.fedoraproject.org/D389
> 
> As I already mentioned in the review, the way how Phab displays lint
> errors is a problem in this particular case - the diff is hard to
> read. That is something I'd like to avoid in the future.

Getting rid of most lint errors/warnings/advice would be one way of
doing that :)
 
> > > 
> > > Since the last discussion about coding style ended in a long
> > > discussion about PEP8, the bits we may not like about the
> > > standard and the exceptions that we'd want, I'm proposing that we
> > > use strict PEP8 with almost no exceptions. 
> 
> The big hammer solution! ;-)

I prefer to think of it as the simple hammer solution ... that
happens to be big :)

> > > I'm not saying that that PEP8 is perfect or
> > > that there aren't parts of it I would like to tweak but
> > >  - it's a well known standard, easy for non-regular contributors
> > > to understand and use
> > >  - style checking tools tend to use PEP8 out of the box
> > >  - if we don't allow exceptions, there will be less time spent on
> > >    discussing details instead of being productive :)
> 
> The opposite is true for me. My experience with lint warnings in Phab
> (before we loosened the rules) is that I spent a considerable time
> rewriting and reformatting the code until the damn linter finally
> shut up (I'd probably need stronger words to demonstrate my temper at
> that time:)). Also it made the code slightly less readable sometimes
> (a personal view), which annoyed me. I lose no productivity on seeing
> an occasional lint warning in diff review (D389 is a slight exception
> in a very long time).
> 
> That being said, I try to minimize the number of lint warnings with
> my every patch, unless I consider that specific rule particularly
> braindead.

Running flake8 on code before using arc is one option - using an
editor/ide that has flake8/pep8 checking enabled is another way to
make the change less frustrating.

Again, I'm not saying that I'm thrilled with all the rules in PEP8 -
several of them are going to require me to break some of my habits but
I do think that this will save time and effort in the long run.

> > >
> > > To be more specific, I am proposing the following:
> > >  - all QA devel projects be have flake8 as the linter in arc
> > > config
> > >  - no code be accepted with lint errors unless there is
> > > absolutely no other way to get around it
> > >  - until we get our entire codebase PEP8 compliant, "if you touch
> > > a file, fix the lint errors even if those errors are not part of
> > > what you're changing"
> > > 
> > > Any strong objections to starting this? Any strong objections
> > > should have an alternative proposal and a justification why that
> > > proposal is worth deviating from a well known and established
> > > standard.
> > > 
> 
> I have an alternative proposal:
> 
> 1. The Phab diff should not contain too many lint warnings without a
> good enough reason, in order to keep it readable. 2. If it does, ask
> the author to fix it. 3. If the author is reluctant to change his
> coding style, and you have no strong objection to it per se (you just
> want the Phab diff to be readable), let's disable the particular lint
> check.
> 
> In this case it would involve asking Josef to stop putting spaces
> between parameter keyvals, or disabling "unexpected spaces around
> keyword / parameter equals" check. It could also involve disabling
> "continuation line over-indented for hanging indent" check, because
> that's another very common warning we just ignore.

I'm pretty much at a harder stance than that - unless there is a reason
that the code cannot be refactored to pass lint, it should have 0 lint
errors, warnings or advice. Discussing why a particular message is "ok
this time because ..." costs time and energy that would be avoided with
simpler rules. Code style enforcement is there to make sure that code
is consistently styled - if there are (semi) frequent exceptions to the
rules, what's the point of checking style in the first place?

> The only problem will be when two people have strong and conflicting
> opinions on how to code should look. Is this the case? If not, then
> if we implement the solution describe above, I believe we're not
> going to be discussing code style in another year or two, judging by
> the past.

Which is exactly what I want to avoid by saying "no exceptions unless
unavoidable". I suspect that we all have habits which go against PEP8
and it'll take some relearning of habits but it's not like PEP8 is some
obscure standard which nobody else uses, this won't be the last time
that patches would be required to adhere to that standard.

> > By saying we should use strict PEP8, does that mean you want to get
> > rid of the linter settings we have in .arclint or leave it there?
> 
> Strict PEP8 would mean all the configured exceptions would be purged.

Most, if not all, yes. If we take the route of not allowing exceptions,
I don't think there's a reason to keep the config that's changing
errors to "advice" or "warning" - reviews with advice or warnings
wouldn't be more acceptable than errors and I don't see the difference.

> Oh boy, this will be a long discussion once again.

Which I really want to avoid and why I'm proposing strict PEP8 - fewer
variables and less time spent figuring out what everyone's "issues"
with the standard are.

Tim
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 473 bytes
Desc: OpenPGP digital signature
URL: <http://lists.fedoraproject.org/pipermail/qa-devel/attachments/20150612/89880ce4/attachment.sig>