On Mon, May 5, 2014 at 10:56 AM, Ben Nemec <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.


>
>  - 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.


>
>
>> [1]
>> https://blueprints.launchpad.net/keystone/+spec/more-code-
>> style-automation
>> [2] 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
>>
>> [4] https://review.openstack.org/#/c/87832/
>> <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
>> 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
>
_______________________________________________
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Reply via email to