On 05/06/2014 01:13 PM, Joe Gordon wrote:
On Mon, May 5, 2014 at 10:56 AM, Ben Nemec <openst...@nemebean.com
<mailto:openst...@nemebean.com>> wrote:
On 05/05/2014 10:02 AM, ChangBo Guo wrote:
Hi Stackers,
I find some common code style would be avoided while I'm
reviewing code
,so think these check would
be nice to move into local hacking. The local hacking can ease
reviewer
burden.
The idea from keystone blueprint [1].
Hacking is a great start at automating checks for common style
issues.
There are still lots of things that it is not checking for that it
probably should. The local hacking ease reviewer burden . This
is the
list of from [1][2] that would be nice to move into an automated
check:
- use import style 'from openstack.common.* import' not use 'import
openstack.common.*'
This is the only one that I think belongs in Oslo. The others are
all generally applicable, but the other projects aren't going to
want to enforce the import style since it's only to make Oslo syncs
work right.
I agree this belongs in oslo-incubator only as well. But I am still
confused as to why this is needed. It sounds like this is a workaround
for a bug in the sync script.
I feel like there was a reason we couldn't make both import styles work
properly, but my memory is fuzzy. Maybe ChangBo can comment.
- assertIsNone should be used when using None with assertEqual
- _() should not be used in debug log statements
-do not use 'assertTrue(isinstance(a, b)) sentence'
-do not use 'assertEqual(type(A), B) sentence'
The _() one in particular I think we'll want as we make the logging
changes. Some additional checks to make sure the the correct _
function is used with the correct logging function would be good too
(for example, LOG.warning(_LE('foo')) should fail pep8).
But again, that belongs in hacking proper, not an Oslo module.
Yup, this belongs in hacking although this gets a little tricky to do
right, as we don't want false positives
http://git.openstack.org/cgit/openstack-dev/hacking/tree/README.rst#n42
We have to make sure that '_' is what we think it is and that 'LOG' is
the logger, just checking for 'LOG' isn't very robust.
Patches welcome! http://git.openstack.org/cgit/openstack-dev/hacking
The assert ones do seem to fit the best practices as I understand
them, but I suspect there's going to be quite a bit of work to get
projects compliant.
Although some projects already have the assert ones, I don't see a lot
of value in them.
I would probably give priority to the assertTrue(isinstance...) one
because I believe assertTrue yields a less useful error message when it
fails. The others are mostly style issues I don't feel that strongly
about, although I could see some value in them just to avoid having
someone come along and leave a "-1, use assertIsNone" comment (I try not
to -1 for that alone, but I won't promise that I never have, and I'm
pretty sure other people do).
[1]
https://blueprints.launchpad.__net/keystone/+spec/more-code-__style-automation
<https://blueprints.launchpad.net/keystone/+spec/more-code-style-automation>
[2]
https://github.com/openstack/__nova/blob/master/nova/hacking/__checks.py
<https://github.com/openstack/nova/blob/master/nova/hacking/checks.py>
I just registered a blueprint for this in [3] and submit first
patch in [4].
[3]
https://blueprints.launchpad.__net/oslo/+spec/oslo-local-__hacking
<https://blueprints.launchpad.net/oslo/+spec/oslo-local-hacking>
[4] https://review.openstack.org/#__/c/87832/
<https://review.openstack.org/#/c/87832/>
<https://github.com/openstack/__nova/blob/master/nova/hacking/__checks.py
<https://github.com/openstack/nova/blob/master/nova/hacking/checks.py>>
Should we add local hacking for oslo-incubator ? If yes, what's the
other check will be added ?
Your comment is appreciated :-)
--
ChangBo Guo(gcb)
_________________________________________________
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.__org
<mailto:OpenStack-dev@lists.openstack.org>
http://lists.openstack.org/__cgi-bin/mailman/listinfo/__openstack-dev
<http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev>
_________________________________________________
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.__org
<mailto:OpenStack-dev@lists.openstack.org>
http://lists.openstack.org/__cgi-bin/mailman/listinfo/__openstack-dev
<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
