Open Stack

On 05/06/2014 01:13 PM, Joe Gordon wrote:
>
>
>
> On Mon, May 5, 2014 at 10:56 AM, Ben Nemec <openstack at nemebean.com
> <mailto:openstack at 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 at lists.openstack.__org
>         <mailto:OpenStack-dev at 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 at lists.openstack.__org
>     <mailto:OpenStack-dev at lists.openstack.org>
>     http://lists.openstack.org/__cgi-bin/mailman/listinfo/__openstack-dev <http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev>
>
>