Re: [openstack-dev] [oslo] Proposal: add local hacking for oslo-incubator
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
Re: [openstack-dev] [oslo] Proposal: add local hacking for oslo-incubator
On Mon, May 5, 2014 at 9:06 PM, David Stanek dsta...@dstanek.com wrote: On Mon, May 5, 2014 at 5:28 PM, Doug Hellmann doug.hellm...@dreamhost.com wrote: 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. I've seen some work being done on that already, but I don't know how strongly we care about those specific rules as an overall project. I created the Keystone blueprint[1] to automate the things we already check for in reviews. My motivation was to make it faster for contributors to contribute because they would get feedback before getting a bunch of -1s in Gerrit. I also wanted to free up core dev resources so that we can focus on more important parts of reviews. Sure, that makes sense to do in keystone. I'd be happy to start putting some of these in hacking, but I don't know which rules would be acceptable to all projects. Maybe there is a way to make optional checks that can be enabled in the tox.ini This is the part I wasn't sure about. Some of the rules look pretty useful (e.g., no mutable arguments). Others are more stylistic choices (e.g., which type of quotes to use). Joe Gordon drives the hacking project, and he might be able to give more detail about the process for adding new rules there. Doug 1. https://blueprints.launchpad.net/keystone/+spec/more-code-style-automation -- David blog: http://www.traceback.org twitter: http://twitter.com/dstanek www: http://dstanek.com ___ 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
Re: [openstack-dev] [oslo] Proposal: add local hacking for oslo-incubator
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
Re: [openstack-dev] [oslo] Proposal: add local hacking for oslo-incubator
On Tue, May 6, 2014 at 6:03 AM, Doug Hellmann doug.hellm...@dreamhost.comwrote: On Mon, May 5, 2014 at 9:06 PM, David Stanek dsta...@dstanek.com wrote: On Mon, May 5, 2014 at 5:28 PM, Doug Hellmann doug.hellm...@dreamhost.com wrote: 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. I've seen some work being done on that already, but I don't know how strongly we care about those specific rules as an overall project. I created the Keystone blueprint[1] to automate the things we already check for in reviews. My motivation was to make it faster for contributors to contribute because they would get feedback before getting a bunch of -1s in Gerrit. I also wanted to free up core dev resources so that we can focus on more important parts of reviews. Sure, that makes sense to do in keystone. While local checks are great, when adding them projects should always think if it makes sense to push them back to hacking itself. Otherwise we end up with 10 different versions of the same checks. I'd be happy to start putting some of these in hacking, but I don't know which rules would be acceptable to all projects. Maybe there is a way to make optional checks that can be enabled in the tox.ini All rules are optional in hacking, but all are enabled by default. If a project doesn't want to use one they can just disable it. This is the part I wasn't sure about. Some of the rules look pretty useful (e.g., no mutable arguments). Others are more stylistic choices (e.g., which type of quotes to use). Joe Gordon drives the hacking project, and he might be able to give more detail about the process for adding new rules there. Doug 1. https://blueprints.launchpad.net/keystone/+spec/more-code-style-automation Taking the list from your blueprint, with responses after each. - block comments should have a space after the # (this is already enforced for inline comments) - pep8 1.5.x has this check already, it will come out in hacking 0.9 (which is currently blocked on reviews) - mutables should not be used as default args - sounds like a good thing to add to hacking. Although a clear why this is good is required - % should not be used in log statements - Belongs in Hacking, although we have to make sure 'LOG' is a logger - assertNone should be used when using None with assertEqual - Why? what is the value here? - spelling check for comments and docstrings - I have thought about adding this a few times, but I am -1 on it. The only way this can work is as a blacklist of words, since developers love to make up there own words. - Having spelling feedback in a non-voting way would be very useful though - warn if children that change method signature of their parent - Hacking doesn't really do warnings - methods that just pass are useless and should be deleted - How do these even get in? - warn on try-except that just passes - Hacking doesn't do warnings. - enforce import ordering and spacing - So I am really sad to see this here. Hacking 0.9 supports this (once again blocked on reviews -- not enough reviewers) - _() should not be used in debug log statements - Hacking, once again we need to be rigorous in this check when adding to hacking. I'm sure that as I'm fixing the code to conform to the new checks, I'll identify additional checks to implement. (stevemar) if I may add some more: - ensure no vim headers - I don't think this one belongs in hacking, as once a project removes all vim headers I don't think they will be re-added. So I see value of having this short term in each project. But not something we want to check for indefinitely - assertEqual(len(some_list), 0) should be assertEqual(some_list, []) - Why is this better? - methods / functions that are unused - This isn't really something hacking can do. - class properties that are unused - Once again this isn't something hacking can really do, or really any static analysis in python. pyflakes tries to do some of this. - ensure incoming method args are actually used - belongs in Pyflakes - doc strings - ensure all method args are mentioned (if any are mentioned) - I have to double check, but I think there is a docstring module for flake8 that can do things like this - use ' over - Why? And is this a black and white thing that can be enforced. - ensure we don't go assigning copyrights to OpenStack Foundation - We cannot gate on this, otherwise foundation employees cannot commit code. -- David blog: http://www.traceback.org twitter: http://twitter.com/dstanek www: http://dstanek.com ___ OpenStack-dev mailing list
[openstack-dev] [oslo] Proposal: add local hacking for oslo-incubator
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.*' - 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' [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
Re: [openstack-dev] [oslo] Proposal: add local hacking for oslo-incubator
On Mon, May 5, 2014 at 8:02 AM, ChangBo Guo glongw...@gmail.com 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: Can you go into why its worth enforcing each of these? As a rule of thumb I prefer to add checks to hacking when relevant to all projects instead of each project re-inventing a local hacking rule. - use import style 'from openstack.common.* import' not use 'import openstack.common.*' - 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' [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
Re: [openstack-dev] [oslo] Proposal: add local hacking for oslo-incubator
On Mon, May 5, 2014 at 1:56 PM, 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.
+1, and only for the incubator repository
- 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.
+1
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.
I've seen some work being done on that already, but I don't know how
strongly we care about those specific rules as an overall project.
Doug
[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
Re: [openstack-dev] [oslo] Proposal: add local hacking for oslo-incubator
On Mon, May 5, 2014 at 5:28 PM, Doug Hellmann doug.hellm...@dreamhost.comwrote: 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. I've seen some work being done on that already, but I don't know how strongly we care about those specific rules as an overall project. I created the Keystone blueprint[1] to automate the things we already check for in reviews. My motivation was to make it faster for contributors to contribute because they would get feedback before getting a bunch of -1s in Gerrit. I also wanted to free up core dev resources so that we can focus on more important parts of reviews. I'd be happy to start putting some of these in hacking, but I don't know which rules would be acceptable to all projects. Maybe there is a way to make optional checks that can be enabled in the tox.ini 1. https://blueprints.launchpad.net/keystone/+spec/more-code-style-automation -- David blog: http://www.traceback.org twitter: http://twitter.com/dstanek www: http://dstanek.com ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
