Re: [openstack-dev] [hacking] proposed rules drop for 1.0
On 12/16/2014 06:22 PM, Ben Nemec wrote: Some thoughts inline. I'll go ahead and push a change to remove the things everyone seems to agree on. On 12/09/2014 09:05 AM, Sean Dague wrote: On 12/09/2014 09:11 AM, Doug Hellmann wrote: On Dec 9, 2014, at 6:39 AM, Sean Dague s...@dague.net wrote: I'd like to propose that for hacking 1.0 we drop 2 groups of rules entirely. 1 - the entire H8* group. This doesn't function on python code, it functions on git commit message, which makes it tough to run locally. It also would be a reason to prevent us from not rerunning tests on commit message changes (something we could do after the next gerrit update). 2 - the entire H3* group - because of this - https://review.openstack.org/#/c/140168/2/nova/tests/fixtures.py,cm A look at the H3* code shows that it's terribly complicated, and is often full of bugs (a few bit us last week). I'd rather just delete it and move on. I don’t have the hacking rules memorized. Could you describe them briefly? Sure, the H8* group is git commit messages. It's checking for line length in the commit message. - [H802] First, provide a brief summary of 50 characters or less. Summaries of greater then 72 characters will be rejected by the gate. - [H801] The first line of the commit message should provide an accurate description of the change, not just a reference to a bug or blueprint. H802 is mechanically enforced (though not the 50 characters part, so the code isn't the same as the rule). H801 is enforced by a regex that looks to see if the first line is a launchpad bug and fails on it. You can't mechanically enforce that english provides an accurate description. +1. It would be nice to provide automatic notification to people if they submit something with an absurdly long commit message, but I agree that hacking isn't the place to do that. H3* are all the module import rules: Imports --- - [H302] Do not import objects, only modules (*) - [H301] Do not import more than one module per line (*) - [H303] Do not use wildcard ``*`` import (*) - [H304] Do not make relative imports - Order your imports by the full module path - [H305 H306 H307] Organize your imports according to the `Import order template`_ and `Real-world Import Order Examples`_ below. I think these remain reasonable guidelines, but H302 is exceptionally tricky to get right, and we keep not getting it right. H305-307 are actually impossible to get right. Things come in and out of stdlib in python all the time. tdlr; I'd like to remove H302, H305 and, H307 and leave the rest. Reasons below. +1 to H305 and H307. I'm going to have to admit defeat and accept that I can't make them work in a sane fashion. H306 is different though - that one is only checking alphabetical order and only works on the text of the import so it doesn't have the issues around having modules installed or mis-categorizing. AFAIK it has never actually caused any problems either (the H306 failure in https://review.openstack.org/#/c/140168/2/nova/tests/unit/test_fixtures.py is correct - nova.tests.fixtures should come before nova.tests.unit.conf_fixture). The issue I originally had was in nova.tests.fixtures, where it resolved fixtures as a relative import instead of an absolute one, and exploded. It's not reproducing now though. As far as 301-304, only 302 actually depends on the is_module stuff. The others are all text-based too so I think we should leave them. H302 I'm kind of indifferent on - we hit an edge case with the olso namespace thing which is now fixed, but if removing that allows us to not install requirements.txt to run pep8 I think I'm onboard with removing it too. H304 needs is_import_exception. is_module and is_import_exception means we have to import all the code, which means the depends for pep8 is *all* of requirements.txt, all of test-requirements.txt all of any optional (not listed in those requirements). If the content isn't in the venv, the check passes. So adding / removing an optional requirement can change the flake8 test results. Evaluating the code is something that we should avoid. -Sean -- Sean Dague http://dague.net ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [hacking] proposed rules drop for 1.0
For anyone who's interested, the final removals are in a series starting here: https://review.openstack.org/#/c/142585/ On 12/09/2014 05:39 AM, Sean Dague wrote: I'd like to propose that for hacking 1.0 we drop 2 groups of rules entirely. 1 - the entire H8* group. This doesn't function on python code, it functions on git commit message, which makes it tough to run locally. It also would be a reason to prevent us from not rerunning tests on commit message changes (something we could do after the next gerrit update). 2 - the entire H3* group - because of this - https://review.openstack.org/#/c/140168/2/nova/tests/fixtures.py,cm A look at the H3* code shows that it's terribly complicated, and is often full of bugs (a few bit us last week). I'd rather just delete it and move on. -Sean ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [hacking] proposed rules drop for 1.0
Some thoughts inline. I'll go ahead and push a change to remove the things everyone seems to agree on. On 12/09/2014 09:05 AM, Sean Dague wrote: On 12/09/2014 09:11 AM, Doug Hellmann wrote: On Dec 9, 2014, at 6:39 AM, Sean Dague s...@dague.net wrote: I'd like to propose that for hacking 1.0 we drop 2 groups of rules entirely. 1 - the entire H8* group. This doesn't function on python code, it functions on git commit message, which makes it tough to run locally. It also would be a reason to prevent us from not rerunning tests on commit message changes (something we could do after the next gerrit update). 2 - the entire H3* group - because of this - https://review.openstack.org/#/c/140168/2/nova/tests/fixtures.py,cm A look at the H3* code shows that it's terribly complicated, and is often full of bugs (a few bit us last week). I'd rather just delete it and move on. I don’t have the hacking rules memorized. Could you describe them briefly? Sure, the H8* group is git commit messages. It's checking for line length in the commit message. - [H802] First, provide a brief summary of 50 characters or less. Summaries of greater then 72 characters will be rejected by the gate. - [H801] The first line of the commit message should provide an accurate description of the change, not just a reference to a bug or blueprint. H802 is mechanically enforced (though not the 50 characters part, so the code isn't the same as the rule). H801 is enforced by a regex that looks to see if the first line is a launchpad bug and fails on it. You can't mechanically enforce that english provides an accurate description. +1. It would be nice to provide automatic notification to people if they submit something with an absurdly long commit message, but I agree that hacking isn't the place to do that. H3* are all the module import rules: Imports --- - [H302] Do not import objects, only modules (*) - [H301] Do not import more than one module per line (*) - [H303] Do not use wildcard ``*`` import (*) - [H304] Do not make relative imports - Order your imports by the full module path - [H305 H306 H307] Organize your imports according to the `Import order template`_ and `Real-world Import Order Examples`_ below. I think these remain reasonable guidelines, but H302 is exceptionally tricky to get right, and we keep not getting it right. H305-307 are actually impossible to get right. Things come in and out of stdlib in python all the time. tdlr; I'd like to remove H302, H305 and, H307 and leave the rest. Reasons below. +1 to H305 and H307. I'm going to have to admit defeat and accept that I can't make them work in a sane fashion. H306 is different though - that one is only checking alphabetical order and only works on the text of the import so it doesn't have the issues around having modules installed or mis-categorizing. AFAIK it has never actually caused any problems either (the H306 failure in https://review.openstack.org/#/c/140168/2/nova/tests/unit/test_fixtures.py is correct - nova.tests.fixtures should come before nova.tests.unit.conf_fixture). As far as 301-304, only 302 actually depends on the is_module stuff. The others are all text-based too so I think we should leave them. H302 I'm kind of indifferent on - we hit an edge case with the olso namespace thing which is now fixed, but if removing that allows us to not install requirements.txt to run pep8 I think I'm onboard with removing it too. I think it's time to just decide to be reasonable Humans and that these are guidelines. The H3* set of rules is also why you have to install *all* of requirements.txt and test-requirements.txt in your pep8 tox target, because H302 actually inspects the sys.modules to attempt to figure out if things are correct. -Sean Doug - [H802] First, provide a brief summary of 50 characters or less. Summaries of greater then 72 characters will be rejected by the gate. - [H801] The first line of the commit message should provide an accurate description of the change, not just a reference to a bug or blueprint. -Sean -- Sean Dague http://dague.net ___ 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] [hacking] proposed rules drop for 1.0
On Tue, Dec 16, 2014 at 3:22 PM, Ben Nemec openst...@nemebean.com wrote: Some thoughts inline. I'll go ahead and push a change to remove the things everyone seems to agree on. On 12/09/2014 09:05 AM, Sean Dague wrote: On 12/09/2014 09:11 AM, Doug Hellmann wrote: On Dec 9, 2014, at 6:39 AM, Sean Dague s...@dague.net wrote: I'd like to propose that for hacking 1.0 we drop 2 groups of rules entirely. 1 - the entire H8* group. This doesn't function on python code, it functions on git commit message, which makes it tough to run locally. It also would be a reason to prevent us from not rerunning tests on commit message changes (something we could do after the next gerrit update). 2 - the entire H3* group - because of this - https://review.openstack.org/#/c/140168/2/nova/tests/fixtures.py,cm A look at the H3* code shows that it's terribly complicated, and is often full of bugs (a few bit us last week). I'd rather just delete it and move on. I don’t have the hacking rules memorized. Could you describe them briefly? Sure, the H8* group is git commit messages. It's checking for line length in the commit message. - [H802] First, provide a brief summary of 50 characters or less. Summaries of greater then 72 characters will be rejected by the gate. - [H801] The first line of the commit message should provide an accurate description of the change, not just a reference to a bug or blueprint. H802 is mechanically enforced (though not the 50 characters part, so the code isn't the same as the rule). H801 is enforced by a regex that looks to see if the first line is a launchpad bug and fails on it. You can't mechanically enforce that english provides an accurate description. +1. It would be nice to provide automatic notification to people if they submit something with an absurdly long commit message, but I agree that hacking isn't the place to do that. H3* are all the module import rules: Imports --- - [H302] Do not import objects, only modules (*) - [H301] Do not import more than one module per line (*) - [H303] Do not use wildcard ``*`` import (*) - [H304] Do not make relative imports - Order your imports by the full module path - [H305 H306 H307] Organize your imports according to the `Import order template`_ and `Real-world Import Order Examples`_ below. I think these remain reasonable guidelines, but H302 is exceptionally tricky to get right, and we keep not getting it right. H305-307 are actually impossible to get right. Things come in and out of stdlib in python all the time. tdlr; I'd like to remove H302, H305 and, H307 and leave the rest. Reasons below. +1 to H305 and H307. I'm going to have to admit defeat and accept that I can't make them work in a sane fashion. ++, these have been nothing but trouble. H306 is different though - that one is only checking alphabetical order and only works on the text of the import so it doesn't have the issues around having modules installed or mis-categorizing. AFAIK it has never actually caused any problems either (the H306 failure in https://review.openstack.org/#/c/140168/2/nova/tests/unit/test_fixtures.py is correct - nova.tests.fixtures should come before nova.tests.unit.conf_fixture). Agreed H306 is mechanically enforceable and is there in part to reduce the risk of merge conflicts in the imports section As far as 301-304, only 302 actually depends on the is_module stuff. The others are all text-based too so I think we should leave them. H302 I'm kind of indifferent on - we hit an edge case with the olso namespace thing which is now fixed, but if removing that allows us to not install requirements.txt to run pep8 I think I'm onboard with removing it too. As for H302, it comes from https://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Imports#Imports We still don't have this one working right, running flake8 outside a venv is still causing oslo packaging related issues for me ./nova/i18n.py:21:1: H302 import only modules.'from oslo import i18n' does not import a module So +1 to just removing it. I think it's time to just decide to be reasonable Humans and that these are guidelines. The H3* set of rules is also why you have to install *all* of requirements.txt and test-requirements.txt in your pep8 tox target, because H302 actually inspects the sys.modules to attempt to figure out if things are correct. -Sean Doug - [H802] First, provide a brief summary of 50 characters or less. Summaries of greater then 72 characters will be rejected by the gate. - [H801] The first line of the commit message should provide an accurate description of the change, not just a reference to a bug or blueprint. -Sean -- Sean Dague http://dague.net ___ OpenStack-dev mailing list
Re: [openstack-dev] [hacking] proposed rules drop for 1.0
On Tue, Dec 09, 2014 at 06:39:43AM -0500, Sean Dague wrote: I'd like to propose that for hacking 1.0 we drop 2 groups of rules entirely. 1 - the entire H8* group. This doesn't function on python code, it functions on git commit message, which makes it tough to run locally. It also would be a reason to prevent us from not rerunning tests on commit message changes (something we could do after the next gerrit update). -1, We probably want to recommend a git commit message more stronger formatted mainly about the first line which is the most important. It should reflect which part of the code the commit is attended to update that gives the ability for contributors to quickly see on what the submission is related; An example with Nova which is quite big: api, compute, doc, scheduler, virt, vmware, libvirt, objects... We should to use a prefix in the first line of commit message. There is a large number of commits waiting for reviews, that can help contributors with a knowledge in a particular domain to identify quickly which one to pick. 2 - the entire H3* group - because of this - https://review.openstack.org/#/c/140168/2/nova/tests/fixtures.py,cm A look at the H3* code shows that it's terribly complicated, and is often full of bugs (a few bit us last week). I'd rather just delete it and move on. -Sean -- Sean Dague http://dague.net ___ 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] [hacking] proposed rules drop for 1.0
On 12/09/2014 07:32 AM, Sahid Orentino Ferdjaoui wrote: On Tue, Dec 09, 2014 at 06:39:43AM -0500, Sean Dague wrote: I'd like to propose that for hacking 1.0 we drop 2 groups of rules entirely. 1 - the entire H8* group. This doesn't function on python code, it functions on git commit message, which makes it tough to run locally. It also would be a reason to prevent us from not rerunning tests on commit message changes (something we could do after the next gerrit update). -1, We probably want to recommend a git commit message more stronger formatted mainly about the first line which is the most important. It should reflect which part of the code the commit is attended to update that gives the ability for contributors to quickly see on what the submission is related; An example with Nova which is quite big: api, compute, doc, scheduler, virt, vmware, libvirt, objects... We should to use a prefix in the first line of commit message. There is a large number of commits waiting for reviews, that can help contributors with a knowledge in a particular domain to identify quickly which one to pick. And how exactly do you expect a machine to decide if that's done correctly? -Sean 2 - the entire H3* group - because of this - https://review.openstack.org/#/c/140168/2/nova/tests/fixtures.py,cm A look at the H3* code shows that it's terribly complicated, and is often full of bugs (a few bit us last week). I'd rather just delete it and move on. -Sean -- Sean Dague http://dague.net ___ 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 -- Sean Dague http://dague.net ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [hacking] proposed rules drop for 1.0
On Tue, Dec 09 2014, Sean Dague wrote: 1 - the entire H8* group. This doesn't function on python code, it functions on git commit message, which makes it tough to run locally. It also would be a reason to prevent us from not rerunning tests on commit message changes (something we could do after the next gerrit update). +1 2 - the entire H3* group - because of this - https://review.openstack.org/#/c/140168/2/nova/tests/fixtures.py,cm A look at the H3* code shows that it's terribly complicated, and is often full of bugs (a few bit us last week). I'd rather just delete it and move on. -0 Not sure it's a good idea to drop it, but I don't have strong arguments for it. -- Julien Danjou /* Free Software hacker http://julien.danjou.info */ signature.asc Description: PGP signature ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [hacking] proposed rules drop for 1.0
On Dec 9, 2014, at 6:39 AM, Sean Dague s...@dague.net wrote: I'd like to propose that for hacking 1.0 we drop 2 groups of rules entirely. 1 - the entire H8* group. This doesn't function on python code, it functions on git commit message, which makes it tough to run locally. It also would be a reason to prevent us from not rerunning tests on commit message changes (something we could do after the next gerrit update). 2 - the entire H3* group - because of this - https://review.openstack.org/#/c/140168/2/nova/tests/fixtures.py,cm A look at the H3* code shows that it's terribly complicated, and is often full of bugs (a few bit us last week). I'd rather just delete it and move on. I don’t have the hacking rules memorized. Could you describe them briefly? Doug -Sean -- Sean Dague http://dague.net ___ 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] [hacking] proposed rules drop for 1.0
On 12/09/2014 09:11 AM, Doug Hellmann wrote: On Dec 9, 2014, at 6:39 AM, Sean Dague s...@dague.net wrote: I'd like to propose that for hacking 1.0 we drop 2 groups of rules entirely. 1 - the entire H8* group. This doesn't function on python code, it functions on git commit message, which makes it tough to run locally. It also would be a reason to prevent us from not rerunning tests on commit message changes (something we could do after the next gerrit update). 2 - the entire H3* group - because of this - https://review.openstack.org/#/c/140168/2/nova/tests/fixtures.py,cm A look at the H3* code shows that it's terribly complicated, and is often full of bugs (a few bit us last week). I'd rather just delete it and move on. I don’t have the hacking rules memorized. Could you describe them briefly? Sure, the H8* group is git commit messages. It's checking for line length in the commit message. - [H802] First, provide a brief summary of 50 characters or less. Summaries of greater then 72 characters will be rejected by the gate. - [H801] The first line of the commit message should provide an accurate description of the change, not just a reference to a bug or blueprint. H802 is mechanically enforced (though not the 50 characters part, so the code isn't the same as the rule). H801 is enforced by a regex that looks to see if the first line is a launchpad bug and fails on it. You can't mechanically enforce that english provides an accurate description. H3* are all the module import rules: Imports --- - [H302] Do not import objects, only modules (*) - [H301] Do not import more than one module per line (*) - [H303] Do not use wildcard ``*`` import (*) - [H304] Do not make relative imports - Order your imports by the full module path - [H305 H306 H307] Organize your imports according to the `Import order template`_ and `Real-world Import Order Examples`_ below. I think these remain reasonable guidelines, but H302 is exceptionally tricky to get right, and we keep not getting it right. H305-307 are actually impossible to get right. Things come in and out of stdlib in python all the time. I think it's time to just decide to be reasonable Humans and that these are guidelines. The H3* set of rules is also why you have to install *all* of requirements.txt and test-requirements.txt in your pep8 tox target, because H302 actually inspects the sys.modules to attempt to figure out if things are correct. -Sean Doug - [H802] First, provide a brief summary of 50 characters or less. Summaries of greater then 72 characters will be rejected by the gate. - [H801] The first line of the commit message should provide an accurate description of the change, not just a reference to a bug or blueprint. -Sean -- Sean Dague http://dague.net ___ 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 -- Sean Dague http://dague.net ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [hacking] proposed rules drop for 1.0
On Tue, Dec 9, 2014 at 12:39 PM, Sean Dague s...@dague.net wrote: 1 - the entire H8* group. This doesn't function on python code, it functions on git commit message, which makes it tough to run locally. I do run them locally using git-review custom script features which would launch a flake8 before sending the review but I guess it's not a common usage. Chmouel ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [hacking] proposed rules drop for 1.0
On 12/09/2014 03:39 AM, Sean Dague wrote: I'd like to propose that for hacking 1.0 we drop 2 groups of rules entirely. 1 - the entire H8* group. This doesn't function on python code, it functions on git commit message, which makes it tough to run locally. It also would be a reason to prevent us from not rerunning tests on commit message changes (something we could do after the next gerrit update). +1 I DO like something warning about commit subject length ... but maybe that should be a git-review function or something. 2 - the entire H3* group - because of this - https://review.openstack.org/#/c/140168/2/nova/tests/fixtures.py,cm A look at the H3* code shows that it's terribly complicated, and is often full of bugs (a few bit us last week). I'd rather just delete it and move on. +1 flake8 does the important one now - no wildcard imports The others are ones where I find myself dancing to meet the style more often than not. ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [hacking] proposed rules drop for 1.0
On Tue, Dec 9, 2014 at 9:05 AM, Sean Dague s...@dague.net wrote: - [H305 H306 H307] Organize your imports according to the `Import order template`_ and `Real-world Import Order Examples`_ below. I think these remain reasonable guidelines, but H302 is exceptionally tricky to get right, and we keep not getting it right. H305-307 are actually impossible to get right. Things come in and out of stdlib in python all the time. Do you have concrete examples of where this has been an issue? Modules are only added roughly every 18 months and only on the 3.x line as of the middle of 2010 when 2.7.0 was released. Nothing should have left the 2.x line within that time as well, and I don't recall anything having completed a deprecation cycle on the 3.x side. ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [hacking] proposed rules drop for 1.0
On 12/09/2014 11:15 AM, Brian Curtis wrote: On Tue, Dec 9, 2014 at 9:05 AM, Sean Dague s...@dague.net wrote: - [H305 H306 H307] Organize your imports according to the `Import order template`_ and `Real-world Import Order Examples`_ below. I think these remain reasonable guidelines, but H302 is exceptionally tricky to get right, and we keep not getting it right. H305-307 are actually impossible to get right. Things come in and out of stdlib in python all the time. Do you have concrete examples of where this has been an issue? Modules are only added roughly every 18 months and only on the 3.x line as of the middle of 2010 when 2.7.0 was released. Nothing should have left the 2.x line within that time as well, and I don't recall anything having completed a deprecation cycle on the 3.x side. argparse - which is stdlib in 2.7, not in 2.6. So hacking on 2.6 would give different results from 2.7. Less of an issue now that 2.6 support in OpenStack has been dropped for most projects, but it's a very concrete example. This check should run on any version of python and give the same results. It does not, because it queries python to know what's in stdlib vs. not. Having a deprecation cycle isn't the concern here, it's the checks working the same on python 2.7, 3.3, 3.4, 3.5 -Sean -- Sean Dague http://dague.net ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [hacking] proposed rules drop for 1.0
On 2014-12-09 07:29:31 -0800 (-0800), Monty Taylor wrote: I DO like something warning about commit subject length ... but maybe that should be a git-review function or something. [...] How about a hook in Gerrit to refuse commits based on some simple (maybe even project-specific) rules? -- Jeremy Stanley ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [hacking] proposed rules drop for 1.0
On Tue, Dec 09, 2014, Sean Dague s...@dague.net wrote: This check should run on any version of python and give the same results. It does not, because it queries python to know what's in stdlib vs. not. Just to underscore that it's difficult to get right, I found out recently that hacking doesn't do a great job of figuring out what is a standard library. I've installed some libraries in 'develop' mode and recent hacking thinks they are standard libraries and complains about the order. JE ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [hacking] proposed rules drop for 1.0
On Tue, Dec 09, 2014 at 10:15:34AM -0600, Brian Curtin wrote: On Tue, Dec 9, 2014 at 9:05 AM, Sean Dague s...@dague.net wrote: - [H305 H306 H307] Organize your imports according to the `Import order template`_ and `Real-world Import Order Examples`_ below. I think these remain reasonable guidelines, but H302 is exceptionally tricky to get right, and we keep not getting it right. H305-307 are actually impossible to get right. Things come in and out of stdlib in python all the time. Do you have concrete examples of where this has been an issue? Modules are only added roughly every 18 months and only on the 3.x line as of the middle of 2010 when 2.7.0 was released. Nothing should have left the 2.x line within that time as well, and I don't recall anything having completed a deprecation cycle on the 3.x side. I don't have any examples of stdlib removals (and there may not be any) but that isn't the only issue with the import grouping rules. The reverse will also cause issues, adding a library to stdlib which was previously a third-party module. The best example I've found is pathlib which was added to stdlib in 3.4: https://docs.python.org/3/library/pathlib.html but a third-party module on all the previous releases: https://pypi.python.org/pypi/pathlib So, the hacking rule will behave differently depending on which version of python you're running with. There really isn't a way around that, if the rule can't behave consistently and enforce the same behavior between releases we shouldn't be using it. Especially as things are trying to migrate to use python 3 where possible. I've seen proposals to hard code the list of stdlib in the rule to a specific python version which would make the behavior consistent, but I very much opposed to that because it means we're not actually enforcing the correct thing which I think is as big an issue. We don't want the hacking checks to error out and say that pathlib is a 3rd party module even if we're running it on python 3.4, that would just be very confusing. The middle ground I proposed was to not differentiate the third-party and stdlib import groups and just check local project's import grouping against the others. This would make the behavior consistent between python versions and still provide some useful feedback. But if the consensus is to just remove the rules I'm fine with that too. -Matt Treinish pgpQEUl84Nl7Y.pgp Description: PGP signature ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [hacking] proposed rules drop for 1.0
On Tue, Dec 09, 2014, Sean Dague s...@dague.net wrote: I'd like to propose that for hacking 1.0 we drop 2 groups of rules entirely. 1 - the entire H8* group. This doesn't function on python code, it functions on git commit message, which makes it tough to run locally. It also would be a reason to prevent us from not rerunning tests on commit message changes (something we could do after the next gerrit update). One of the problems with the H8* tests is that it can reject a commit message generated by git itself. I had a 'git revert' rejected because the first line was too long :( JE ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [hacking] proposed rules drop for 1.0
On 12/09/2014 11:41 AM, Johannes Erdfelt wrote: On Tue, Dec 09, 2014, Sean Dague s...@dague.net wrote: I'd like to propose that for hacking 1.0 we drop 2 groups of rules entirely. 1 - the entire H8* group. This doesn't function on python code, it functions on git commit message, which makes it tough to run locally. It also would be a reason to prevent us from not rerunning tests on commit message changes (something we could do after the next gerrit update). One of the problems with the H8* tests is that it can reject a commit message generated by git itself. I had a 'git revert' rejected because the first line was too long :( +1 I've had the gerrit revert button reject me for the same reason. -Sean -- Sean Dague http://dague.net ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [hacking] proposed rules drop for 1.0
On 12/09/2014 11:28 AM, Jeremy Stanley wrote: On 2014-12-09 07:29:31 -0800 (-0800), Monty Taylor wrote: I DO like something warning about commit subject length ... but maybe that should be a git-review function or something. [...] How about a hook in Gerrit to refuse commits based on some simple (maybe even project-specific) rules? Honestly, any hard rejection ends up problematic. For instance, it means it's impossible to include actual urls in commit messages to reference things without a url shortener much of the time. -Sean -- Sean Dague http://dague.net ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [hacking] proposed rules drop for 1.0
On Tue, 2014-12-09 at 10:05 -0500, Sean Dague wrote: Sure, the H8* group is git commit messages. It's checking for line length in the commit message. I agree the H8* group should be dropped. It would be appropriate to create a new gate check job that validated that, but it should not be part of hacking. H3* are all the module import rules: Imports --- - [H302] Do not import objects, only modules (*) - [H301] Do not import more than one module per line (*) - [H303] Do not use wildcard ``*`` import (*) - [H304] Do not make relative imports - Order your imports by the full module path - [H305 H306 H307] Organize your imports according to the `Import order template`_ and `Real-world Import Order Examples`_ below. I think these remain reasonable guidelines, but H302 is exceptionally tricky to get right, and we keep not getting it right. H305-307 are actually impossible to get right. Things come in and out of stdlib in python all the time. I think it's time to just decide to be reasonable Humans and that these are guidelines. The H3* set of rules is also why you have to install *all* of requirements.txt and test-requirements.txt in your pep8 tox target, because H302 actually inspects the sys.modules to attempt to figure out if things are correct. I agree that dropping H302 and the grouping checks makes sense. I think we should keep the H301, H303, H304, and the basic ordering checks, however; it doesn't seem to me that these would be that difficult to implement or maintain. -- Kevin L. Mitchell kevin.mitch...@rackspace.com Rackspace ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [hacking] proposed rules drop for 1.0
On 12/09/2014 11:58 AM, Kevin L. Mitchell wrote: On Tue, 2014-12-09 at 10:05 -0500, Sean Dague wrote: Sure, the H8* group is git commit messages. It's checking for line length in the commit message. I agree the H8* group should be dropped. It would be appropriate to create a new gate check job that validated that, but it should not be part of hacking. H3* are all the module import rules: Imports --- - [H302] Do not import objects, only modules (*) - [H301] Do not import more than one module per line (*) - [H303] Do not use wildcard ``*`` import (*) - [H304] Do not make relative imports - Order your imports by the full module path - [H305 H306 H307] Organize your imports according to the `Import order template`_ and `Real-world Import Order Examples`_ below. I think these remain reasonable guidelines, but H302 is exceptionally tricky to get right, and we keep not getting it right. H305-307 are actually impossible to get right. Things come in and out of stdlib in python all the time. I think it's time to just decide to be reasonable Humans and that these are guidelines. The H3* set of rules is also why you have to install *all* of requirements.txt and test-requirements.txt in your pep8 tox target, because H302 actually inspects the sys.modules to attempt to figure out if things are correct. I agree that dropping H302 and the grouping checks makes sense. I think we should keep the H301, H303, H304, and the basic ordering checks, however; it doesn't seem to me that these would be that difficult to implement or maintain. Well, be careful what you think is easy - https://github.com/openstack-dev/hacking/blob/master/hacking/checks/imports.py :) -Sean -- Sean Dague http://dague.net ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [hacking] proposed rules drop for 1.0
On 2014-12-09 11:56:54 -0500 (-0500), Sean Dague wrote: Honestly, any hard rejection ends up problematic. For instance, it means it's impossible to include actual urls in commit messages to reference things without a url shortener much of the time. Fair enough. I think this makes it a human problem which we're not going to solve by applying more technology. Drop all of H8XX, make Gerrit preserve votes on commit-message-only patchset updates, decree no more commit message -1s from reviewers, and make it socially acceptable to just edit commit messages of changes you review to bring them up to acceptable standards. -- Jeremy Stanley ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [hacking] proposed rules drop for 1.0
On Tue, 2014-12-09 at 12:05 -0500, Sean Dague wrote: I agree that dropping H302 and the grouping checks makes sense. I think we should keep the H301, H303, H304, and the basic ordering checks, however; it doesn't seem to me that these would be that difficult to implement or maintain. Well, be careful what you think is easy - https://github.com/openstack-dev/hacking/blob/master/hacking/checks/imports.py :) So, hacking_import_rules() is very complex. However, it implements H302 as well as H301, H303, and H304. I feel it can be simplified to just a textual match rule if we remove the H302 implementation: H301 just needs to exclude imports with ',', H303 needs to exclude imports with '*', and H304 is already implemented as a regular expression match. It looks like the basic ordering check I was referring to is H306, which isn't all that complicated. It seems like the rest of the code is related to the checks which I just agreed should be dropped :) Am I missing anything? -- Kevin L. Mitchell kevin.mitch...@rackspace.com Rackspace ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [hacking] proposed rules drop for 1.0
On Dec 9, 2014, at 10:05 AM, Sean Dague s...@dague.net wrote: On 12/09/2014 09:11 AM, Doug Hellmann wrote: On Dec 9, 2014, at 6:39 AM, Sean Dague s...@dague.net wrote: I'd like to propose that for hacking 1.0 we drop 2 groups of rules entirely. 1 - the entire H8* group. This doesn't function on python code, it functions on git commit message, which makes it tough to run locally. It also would be a reason to prevent us from not rerunning tests on commit message changes (something we could do after the next gerrit update). 2 - the entire H3* group - because of this - https://review.openstack.org/#/c/140168/2/nova/tests/fixtures.py,cm A look at the H3* code shows that it's terribly complicated, and is often full of bugs (a few bit us last week). I'd rather just delete it and move on. I don’t have the hacking rules memorized. Could you describe them briefly? Sure, the H8* group is git commit messages. It's checking for line length in the commit message. - [H802] First, provide a brief summary of 50 characters or less. Summaries of greater then 72 characters will be rejected by the gate. - [H801] The first line of the commit message should provide an accurate description of the change, not just a reference to a bug or blueprint. H802 is mechanically enforced (though not the 50 characters part, so the code isn't the same as the rule). H801 is enforced by a regex that looks to see if the first line is a launchpad bug and fails on it. You can't mechanically enforce that english provides an accurate description. Those all seem like things it would be reasonable to drop, especially for the reason you gave that they are frequently not tested locally anyway. H3* are all the module import rules: Imports --- - [H302] Do not import objects, only modules (*) - [H301] Do not import more than one module per line (*) - [H303] Do not use wildcard ``*`` import (*) - [H304] Do not make relative imports - Order your imports by the full module path - [H305 H306 H307] Organize your imports according to the `Import order template`_ and `Real-world Import Order Examples`_ below. I think these remain reasonable guidelines, but H302 is exceptionally tricky to get right, and we keep not getting it right. I definitely agree with that. I thought we had it right now, but maybe there’s still a case where it’s broken? In any case, I’d like to be able to make the Oslo namespace changes API compatible without worrying about if they are hacking-rule-compatible. That does get pretty ugly. H305-307 are actually impossible to get right. Things come in and out of stdlib in python all the time. +1 I think it's time to just decide to be reasonable Humans and that these are guidelines. I assume we have the guidelines written down in the review instructions somewhere already, if they are implemented in hacking? The H3* set of rules is also why you have to install *all* of requirements.txt and test-requirements.txt in your pep8 tox target, because H302 actually inspects the sys.modules to attempt to figure out if things are correct. Yeah, that’s pretty gross. Doug -Sean Doug - [H802] First, provide a brief summary of 50 characters or less. Summaries of greater then 72 characters will be rejected by the gate. - [H801] The first line of the commit message should provide an accurate description of the change, not just a reference to a bug or blueprint. -Sean -- Sean Dague http://dague.net ___ 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 -- Sean Dague http://dague.net ___ 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] [hacking] proposed rules drop for 1.0
On 12/09/2014 12:20 PM, Kevin L. Mitchell wrote: On Tue, 2014-12-09 at 12:05 -0500, Sean Dague wrote: I agree that dropping H302 and the grouping checks makes sense. I think we should keep the H301, H303, H304, and the basic ordering checks, however; it doesn't seem to me that these would be that difficult to implement or maintain. Well, be careful what you think is easy - https://github.com/openstack-dev/hacking/blob/master/hacking/checks/imports.py :) So, hacking_import_rules() is very complex. However, it implements H302 as well as H301, H303, and H304. I feel it can be simplified to just a textual match rule if we remove the H302 implementation: H301 just needs to exclude imports with ',', H303 needs to exclude imports with '*', and H304 is already implemented as a regular expression match. It looks like the basic ordering check I was referring to is H306, which isn't all that complicated. It seems like the rest of the code is related to the checks which I just agreed should be dropped :) Am I missing anything? Yes, the following fails H305 and H306. nova/tests/fixtures.py Fixtures for Nova tests. from __future__ import absolute_import import gettext import logging import os import uuid import fixtures from oslo.config import cfg from nova.db import migration from nova.db.sqlalchemy import api as session from nova import service Because name normalization is hard (fixtures is normalized to nova.tests.fixtures so H305 thinks it should be in group 3, and H306 thinks it should be after oslo.config import cfg). To sort things you have to normalize them. -Sean -- Sean Dague http://dague.net ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [hacking] proposed rules drop for 1.0
On 12/09/2014 12:07 PM, Jeremy Stanley wrote: On 2014-12-09 11:56:54 -0500 (-0500), Sean Dague wrote: Honestly, any hard rejection ends up problematic. For instance, it means it's impossible to include actual urls in commit messages to reference things without a url shortener much of the time. Fair enough. I think this makes it a human problem which we're not going to solve by applying more technology. Drop all of H8XX, make Gerrit preserve votes on commit-message-only patchset updates, decree no more commit message -1s from reviewers, and make it socially acceptable to just edit commit messages of changes you review to bring them up to acceptable standards. I still think -1 for commit message is fine, but it's a human thing, not a computer thing. Because the consumers of the commit messages are humans. And I also think that if a commit message change doesn't retrigger all the tests, people will be a lot happier updating them. -Sean -- Sean Dague http://dague.net ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [hacking] proposed rules drop for 1.0
On 2014-12-09 13:49:00 -0500 (-0500), Sean Dague wrote: [...] And I also think that if a commit message change doesn't retrigger all the tests, people will be a lot happier updating them. Agreed--though this will need a newer Gerrit plus a new feature in Zuul so it recognizes the difference in the stream. -- Jeremy Stanley ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [hacking] proposed rules drop for 1.0
On 12/09/2014 02:46 PM, Jeremy Stanley wrote: On 2014-12-09 13:49:00 -0500 (-0500), Sean Dague wrote: [...] And I also think that if a commit message change doesn't retrigger all the tests, people will be a lot happier updating them. Agreed--though this will need a newer Gerrit plus a new feature in Zuul so it recognizes the difference in the stream. Yes, it's not a tomorrow thing (I know we need Gerrit 2.9 first). But I think it's the way we should evolve the system. -Sean -- Sean Dague http://dague.net ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [hacking] proposed rules drop for 1.0
On Tue, 2014-12-09 at 13:46 -0500, Sean Dague wrote: Yes, the following fails H305 and H306. nova/tests/fixtures.py Fixtures for Nova tests. from __future__ import absolute_import import gettext import logging import os import uuid import fixtures from oslo.config import cfg from nova.db import migration from nova.db.sqlalchemy import api as session from nova import service Because name normalization is hard (fixtures is normalized to nova.tests.fixtures so H305 thinks it should be in group 3, and H306 thinks it should be after oslo.config import cfg). To sort things you have to normalize them. I agree you have to normalize imports to sort them, but to my mind the appropriate normalization here is purely textual; we shouldn't be expecting any relative imports (and should raise an error if there are any). Still, that does show that some work needs to be done to the simpler H306 test (probably involving changes to the core import normalization)… -- Kevin L. Mitchell kevin.mitch...@rackspace.com Rackspace ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
