Re: [openstack-dev] H302 considered harmful
On Wed, 25 Feb 2015 15:47:46 -0500 Doug Hellmann d...@doughellmann.com wrote: I think the rule originally came from the way mock works. If you import a thing in your module and then a test tries to mock where it came from, your module still uses the version it imported because the name lookup isn't done again at the point when the test runs. If all external symbols are accessed through the module that contains them, then the lookup is done at runtime instead of import time and mocks can replace the symbols. The same benefit would apply to monkey patching like what eventlet does, though that's less likely to come up in our own code than it is for third-party and stdlib modules. I strongly agree, when source code does from module import SomeClass is a nightmare patch it at runtime. Those were my 2 cents to the discussion :) -- Felipe Reyes (GPG:0x9B1FFF39) http://tty.cl lp:~freyes | freyes@freenode | freyes@github __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] H302 considered harmful
So, Swift doesn't enforce H302 - and our imports are sorta messy frankly - but it doesn't really bother me, and I do rather enjoy the terseness of not having to spell out the module name. It's not really a chore to maintain, if you don't know where a name came from split the window (or drop a marker) and pop up to the top of the file and there it is - mystery solved. But I've been living in the code base to long to say if hurts new-comers trying to grok where things are coming from. I'd be willing to entertain feedback on this. But one thing that I'd really love to hear feedback on is if people using H302 ever find it's inconvenient to enforce the rule *all the time*? Particularly in stdlib where it'd be *such* bad form to collide with a common name like `defaultdict` or `datetime` anyway, if you see on of those names without the module - you *know* where it came from (hopefully?): * `collections.defaultdict(collections.defaultdict(list))` - no thank you * `datetime.datetime - meh Anyway, every time I start some project greenfield I try to make myself H302, (i *do* get so sick of is it time.time() or time() in this file?) - but I normally break down as soon as I get to a name I'd rather just be be in my right there in my globals... @contextlib.contextmanager, functools.partial, itertools.ifilter - maybe it's just stdlib names? Not sure if there's any compromise, probably better to either *just import modules*, or live with the inconsistency (you eventually get nose-blind to do it ;P) -Clay On Wed, Feb 25, 2015 at 10:51 AM, Duncan Thomas duncan.tho...@gmail.com wrote: Hi So a review [1] was recently submitted to cinder to fix up all of the H302 violations, and turn on the automated check for them. This is certainly a reasonable suggestion given the number of manual reviews that -1 for this issue, however I'm far from convinced it actually makes the code more readable, Is there anybody who'd like to step forward in defence of this rule and explain why it is an improvement? I don't discount for a moment the possibility I'm missing something, and welcome the education in that case Thanks [1] https://review.openstack.org/#/c/145780/ -- Duncan Thomas __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] H302 considered harmful
On Wed, Feb 25, 2015 at 9:47 PM, Doug Hellmann d...@doughellmann.com wrote: On Wed, Feb 25, 2015, at 02:59 PM, Robert Collins wrote: On 26 February 2015 at 08:54, melanie witt melwi...@gmail.com wrote: On Feb 25, 2015, at 10:51, Duncan Thomas duncan.tho...@gmail.com wrote: Is there anybody who'd like to step forward in defence of this rule and explain why it is an improvement? I don't discount for a moment the possibility I'm missing something, and welcome the education in that case A reason I can think of would be to preserve namespacing (no possibility of function or class name collision upon import). Another reason could be maintainability, scenario being: Person 1 imports ClassA from a module to use, Person 2 comes along later and needs a different class from the module so they import ClassB from the same module to use, and it continues. If only the module had been imported, everybody can just do module.ClassA, module.ClassB instead of potentially multiple imports from the same module of different classes and functions. I've also read it doesn't cost more to import the entire module rather than just a function or a class, as the whole module has to be parsed either way. I think the primary benefit is that when looking at the code you can tell where a name came from. If the code is using libraries that one is not familiar with, this makes finding the implementation a lot easier (than e.g. googling it and hoping its unique and not generic like 'create' or something. I think the rule originally came from the way mock works. If you import a thing in your module and then a test tries to mock where it came from, your module still uses the version it imported because the name lookup isn't done again at the point when the test runs. If all external symbols are accessed through the module that contains them, then the lookup is done at runtime instead of import time and mocks can replace the symbols. The same benefit would apply to monkey patching like what eventlet does, though that's less likely to come up in our own code than it is for third-party and stdlib modules. I second Doug's analysis. I've already had a hard time figuring out why mock wasn't doing what I wanted it to do and following H302 just fixed it... BR, Simon Doug -Rob -- Robert Collins rbtcoll...@hp.com Distinguished Technologist HP Converged Cloud __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] H302 considered harmful
On 26 February 2015 at 08:54, melanie witt melwi...@gmail.com wrote: On Feb 25, 2015, at 10:51, Duncan Thomas duncan.tho...@gmail.com wrote: Is there anybody who'd like to step forward in defence of this rule and explain why it is an improvement? I don't discount for a moment the possibility I'm missing something, and welcome the education in that case A reason I can think of would be to preserve namespacing (no possibility of function or class name collision upon import). Another reason could be maintainability, scenario being: Person 1 imports ClassA from a module to use, Person 2 comes along later and needs a different class from the module so they import ClassB from the same module to use, and it continues. If only the module had been imported, everybody can just do module.ClassA, module.ClassB instead of potentially multiple imports from the same module of different classes and functions. I've also read it doesn't cost more to import the entire module rather than just a function or a class, as the whole module has to be parsed either way. I think the primary benefit is that when looking at the code you can tell where a name came from. If the code is using libraries that one is not familiar with, this makes finding the implementation a lot easier (than e.g. googling it and hoping its unique and not generic like 'create' or something. -Rob -- Robert Collins rbtcoll...@hp.com Distinguished Technologist HP Converged Cloud __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] H302 considered harmful
On Wed, Feb 25, 2015 at 12:59 PM, Robert Collins robe...@robertcollins.net wrote: On 26 February 2015 at 08:54, melanie witt melwi...@gmail.com wrote: On Feb 25, 2015, at 10:51, Duncan Thomas duncan.tho...@gmail.com wrote: Is there anybody who'd like to step forward in defence of this rule and explain why it is an improvement? I don't discount for a moment the possibility I'm missing something, and welcome the education in that case A reason I can think of would be to preserve namespacing (no possibility of function or class name collision upon import). Another reason could be maintainability, scenario being: Person 1 imports ClassA from a module to use, Person 2 comes along later and needs a different class from the module so they import ClassB from the same module to use, and it continues. If only the module had been imported, everybody can just do module.ClassA, module.ClassB instead of potentially multiple imports from the same module of different classes and functions. I've also read it doesn't cost more to import the entire module rather than just a function or a class, as the whole module has to be parsed either way. I think the primary benefit is that when looking at the code you can tell where a name came from. If the code is using libraries that one is not familiar with, this makes finding the implementation a lot easier (than e.g. googling it and hoping its unique and not generic like 'create' or something. -Rob -- Robert Collins rbtcoll...@hp.com Distinguished Technologist HP Converged Cloud __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev I'd echo Melanie and Roberts points as being benefits. I've actually recently encountered the name space collision problem pointed out in Melanies example, but most of all I agree with Roberts points about being explicit. Not to mention we can avoid what we have today where the same module is imported under 5 different names (that's annoying). __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] H302 considered harmful
On Wed, Feb 25, 2015 at 10:51 AM, Duncan Thomas duncan.tho...@gmail.com wrote: Hi So a review [1] was recently submitted to cinder to fix up all of the H302 violations, and turn on the automated check for them. This is certainly a reasonable suggestion given the number of manual reviews that -1 for this issue, however I'm far from convinced it actually makes the code more readable, Is there anybody who'd like to step forward in defence of this rule and explain why it is an improvement? I don't discount for a moment the possibility I'm missing something, and welcome the education in that case H302 originally comes from http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Imports#Imports Thanks [1] https://review.openstack.org/#/c/145780/ -- Duncan Thomas __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] H302 considered harmful
Thanks for that, Joe. I'd say the cons miss 'It looks ugly in places'. On 25 February 2015 at 20:54, Joe Gordon joe.gord...@gmail.com wrote: On Wed, Feb 25, 2015 at 10:51 AM, Duncan Thomas duncan.tho...@gmail.com wrote: Hi So a review [1] was recently submitted to cinder to fix up all of the H302 violations, and turn on the automated check for them. This is certainly a reasonable suggestion given the number of manual reviews that -1 for this issue, however I'm far from convinced it actually makes the code more readable, Is there anybody who'd like to step forward in defence of this rule and explain why it is an improvement? I don't discount for a moment the possibility I'm missing something, and welcome the education in that case H302 originally comes from http://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Imports#Imports Thanks [1] https://review.openstack.org/#/c/145780/ -- Duncan Thomas __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev -- Duncan Thomas __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] H302 considered harmful
On Feb 25, 2015, at 10:51, Duncan Thomas duncan.tho...@gmail.com wrote: Is there anybody who'd like to step forward in defence of this rule and explain why it is an improvement? I don't discount for a moment the possibility I'm missing something, and welcome the education in that case A reason I can think of would be to preserve namespacing (no possibility of function or class name collision upon import). Another reason could be maintainability, scenario being: Person 1 imports ClassA from a module to use, Person 2 comes along later and needs a different class from the module so they import ClassB from the same module to use, and it continues. If only the module had been imported, everybody can just do module.ClassA, module.ClassB instead of potentially multiple imports from the same module of different classes and functions. I've also read it doesn't cost more to import the entire module rather than just a function or a class, as the whole module has to be parsed either way. melanie (melwitt) signature.asc Description: Message signed with OpenPGP using GPGMail __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
[openstack-dev] H302 considered harmful
Hi So a review [1] was recently submitted to cinder to fix up all of the H302 violations, and turn on the automated check for them. This is certainly a reasonable suggestion given the number of manual reviews that -1 for this issue, however I'm far from convinced it actually makes the code more readable, Is there anybody who'd like to step forward in defence of this rule and explain why it is an improvement? I don't discount for a moment the possibility I'm missing something, and welcome the education in that case Thanks [1] https://review.openstack.org/#/c/145780/ -- Duncan Thomas __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] H302 considered harmful
Excerpts from Duncan Thomas's message of 2015-02-25 10:51:00 -0800: Hi So a review [1] was recently submitted to cinder to fix up all of the H302 violations, and turn on the automated check for them. This is certainly a reasonable suggestion given the number of manual reviews that -1 for this issue, however I'm far from convinced it actually makes the code more readable, Is there anybody who'd like to step forward in defence of this rule and explain why it is an improvement? I don't discount for a moment the possibility I'm missing something, and welcome the education in that case I think we've had this conclusion a few times before, but let me resurrect it: The reason we have hacking and flake8 and pep8 and etc. etc. is so that code reviews don't descend into nit picking and style spraying. I'd personally have a private conversation with anyone who mentioned this, or any other rule that is in hacking/etc., in a review. I want to know why people think it is a good idea to bombard users with rules that are already called out explicitly in automation. Let the robots do their job, and they will let you do yours (until the singularity, at which point your job will be hiding from the robots). __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] H302 considered harmful
On Wed, Feb 25, 2015, at 02:59 PM, Robert Collins wrote: On 26 February 2015 at 08:54, melanie witt melwi...@gmail.com wrote: On Feb 25, 2015, at 10:51, Duncan Thomas duncan.tho...@gmail.com wrote: Is there anybody who'd like to step forward in defence of this rule and explain why it is an improvement? I don't discount for a moment the possibility I'm missing something, and welcome the education in that case A reason I can think of would be to preserve namespacing (no possibility of function or class name collision upon import). Another reason could be maintainability, scenario being: Person 1 imports ClassA from a module to use, Person 2 comes along later and needs a different class from the module so they import ClassB from the same module to use, and it continues. If only the module had been imported, everybody can just do module.ClassA, module.ClassB instead of potentially multiple imports from the same module of different classes and functions. I've also read it doesn't cost more to import the entire module rather than just a function or a class, as the whole module has to be parsed either way. I think the primary benefit is that when looking at the code you can tell where a name came from. If the code is using libraries that one is not familiar with, this makes finding the implementation a lot easier (than e.g. googling it and hoping its unique and not generic like 'create' or something. I think the rule originally came from the way mock works. If you import a thing in your module and then a test tries to mock where it came from, your module still uses the version it imported because the name lookup isn't done again at the point when the test runs. If all external symbols are accessed through the module that contains them, then the lookup is done at runtime instead of import time and mocks can replace the symbols. The same benefit would apply to monkey patching like what eventlet does, though that's less likely to come up in our own code than it is for third-party and stdlib modules. Doug -Rob -- Robert Collins rbtcoll...@hp.com Distinguished Technologist HP Converged Cloud __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] H302 considered harmful
Clint This rule is not currently enabled in Cinder. This review fixes up all cases and enables it, which is absolutely 100% the right thing to do if we decide to implement this rule. The purpose of this thread is to understand the value of the rule. We should either enforce it, or else explicitly decide to ignore it, and educate reviewers who manually comment on it. I lean against the rule, but there are certainly enough comments coming in that I'll look and think again, which is a good result for the thread. On 25 February 2015 at 22:46, Clint Byrum cl...@fewbar.com wrote: Excerpts from Duncan Thomas's message of 2015-02-25 10:51:00 -0800: Hi So a review [1] was recently submitted to cinder to fix up all of the H302 violations, and turn on the automated check for them. This is certainly a reasonable suggestion given the number of manual reviews that -1 for this issue, however I'm far from convinced it actually makes the code more readable, Is there anybody who'd like to step forward in defence of this rule and explain why it is an improvement? I don't discount for a moment the possibility I'm missing something, and welcome the education in that case I think we've had this conclusion a few times before, but let me resurrect it: The reason we have hacking and flake8 and pep8 and etc. etc. is so that code reviews don't descend into nit picking and style spraying. I'd personally have a private conversation with anyone who mentioned this, or any other rule that is in hacking/etc., in a review. I want to know why people think it is a good idea to bombard users with rules that are already called out explicitly in automation. Let the robots do their job, and they will let you do yours (until the singularity, at which point your job will be hiding from the robots). __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev -- Duncan Thomas __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] H302 considered harmful
Excerpts from Duncan Thomas's message of 2015-02-25 12:51:35 -0800: Clint This rule is not currently enabled in Cinder. This review fixes up all cases and enables it, which is absolutely 100% the right thing to do if we decide to implement this rule. The purpose of this thread is to understand the value of the rule. We should either enforce it, or else explicitly decide to ignore it, and educate reviewers who manually comment on it. I lean against the rule, but there are certainly enough comments coming in that I'll look and think again, which is a good result for the thread. Thanks for your thoughts Duncan, they are appreciated. I believe that what's being missed here is arguing for or against the rule, or even taking time to try and understand it, is far more costly than simply following it if it is enabled or ignoring it if it is not enabled. I don't think any of us want to be project historians, so we should just make sure to have a good commit message when we turn it on or off, and otherwise move forward with the actual development of OpenStack. __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
