On 2014-01-22 22:13, Zane Bitter wrote:
On 22/01/14 06:23, Pavlo Shchelokovskyy wrote:
Hi all,
we have an approved blueprint that concerns reducing number of ignored
PEP8 and openstack/hacking style checks for heat
(https://blueprints.launchpad.net/heat/+spec/reduce-flake8-ignored-rules).
I've been already warned that enabling some of these rules will be
quite
controversial, and personally I do not like some of these rules myself
either. In order to understand what is the opinion of the community, I
would like to ask you to leave a comment on the blueprint page about
what do you think about enabling these checks.
The style rules being currently ignored are:
F841 local variable 'json_template' is assigned to but never used
It would be interesting to do an audit and find out how many of these
are actual dead code and how many are e.g. legitimate discarding of
returned tuple components. If it's almost exclusively the former,
fine. However, for the latter there's no obvious substitute that
doesn't make the code considerably worse, so if the frequency of that
is at all significant then this is probably counter-productive.
It's rather unfortunate that the convention for "discard this tuple
element" is _, which conflicts with translation code if you happen to
have any translated strings that show up after the discarded _ is
assigned. The only solution I see is to use a different convention for
hacking to indicate that a variable is throwaway. Unfortunately this
isn't one of our hacking checks so I think we'd have to reimplement the
flake8 check to recognize the new convention. :-/
H201 no 'except:' at least use 'except Exception:' (this actually
checks
for bare 'except:' lines, so 'except BaseException:' will pass too)
I don't think this check is sophisticated enough. A bare "except:" is
legit provided that it always re-raises the exception. I believe we
already got rid of all the non-legitimate uses, and the code review
process has proven adequate to keep them out (somebody always asks
about it, even if it is legit).
(We have bigger problems with blanket "except Exception:" catching,
but those are even harder again to detect.)
H302 do not import objects, only modules (this I don't like myself as
it
can clutter the code beyond reasonable limit)
I would actually like to see this one happen. We're still adding new
violations of this, often without any particularly strong reason, and
code review doesn't seem to be sufficient to keep them out.
H306 imports not in alphabetical order
This would be nice too. It's hard to know where to put stuff when it's
all jumbled up already.
H404 multi line docstring should start with a summary
This is a massive task for not a lot of benefit IMHO. I think at one
point there was an attempt to do this automatically, but at the end of
the day sticking a random full stop + newline in the middle of the
first sentence of every docstring does not really do anything to make
the world a better place.
Another question I have is how to proceed with such changes. I've
already implemented H306 (order of imports) and am being now puzzled
with how to propose such change to Gerrit. This change naturally
touches
many files (163 so far) and as such is clearly not suited for review
in
one piece. The only solution I currently can think of is to split it
in
4-5-6 patches without actually changing tox.ini, and after all of them
are merged, issue a final patch that updates tox.ini and any files
breaking the rule that were introduced in between. But there is still
a
question on how Jenkins works with verify and merge jobs. Can it
happen
that we end up with code in master that does not pass pep8 check? Or
there will be a 'race condition' between my final patch and any other
that breaks the style rules? I would really appreciate any thoughts
and
comments about this.
No, there's no race condition. Every patch has to pass the gate after
being merged before it is delivered.
cheers,
Zane.
_______________________________________________
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
_______________________________________________
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
