Re: [Txawsteam] [Merge] lp:~oubiwann/txaws/413738-method-name-cleanup into lp:txaws
> in which case, > review +1 Thanks! -- https://code.edge.launchpad.net/~oubiwann/txaws/413738-method-name-cleanup/+merge/10245 Your team txAWS Team is subscribed to branch lp:txaws. ___ Mailing list: https://launchpad.net/~txawsteam Post to : txawsteam@lists.launchpad.net Unsubscribe : https://launchpad.net/~txawsteam More help : https://help.launchpad.net/ListHelp
Re: [Txawsteam] [Merge] lp:~oubiwann/txaws/413738-method-name-cleanup into lp:txaws
in which case, review +1 -- https://code.launchpad.net/~oubiwann/txaws/413738-method-name-cleanup/+merge/10245 Your team txAWS Team is subscribed to branch lp:txaws. ___ Mailing list: https://launchpad.net/~txawsteam Post to : txawsteam@lists.launchpad.net Unsubscribe : https://launchpad.net/~txawsteam More help : https://help.launchpad.net/ListHelp
Re: [Txawsteam] [Merge] lp:~oubiwann/txaws/413738-method-name-cleanup into lp:txaws
A quick ping on this branch... I've got dependent branches starting to pile up... Thanks! -- https://code.edge.launchpad.net/~oubiwann/txaws/413738-method-name-cleanup/+merge/10245 Your team txAWS Team is subscribed to branch lp:txaws. ___ Mailing list: https://launchpad.net/~txawsteam Post to : txawsteam@lists.launchpad.net Unsubscribe : https://launchpad.net/~txawsteam More help : https://help.launchpad.net/ListHelp
Re: [Txawsteam] [Merge] lp:~oubiwann/txaws/413738-method-name-cleanup into lp:txaws
> On Thu, 2009-08-20 at 02:06 +, Duncan McGreggor wrote: > > On Wed, Aug 19, 2009 at 10:46 PM, Robert > > Collins wrote: > > > Uhm, I'm not sure :) > > > > > > The only remaining thing was import sorting. I don't think you replied > > > to my reply on that. > > > > Hrm, well I don't know what you're sorting one... can you explain it? > > the module name, and the item. > > from foo import bar > from gam import alpha, beta > from gam.quux import zulu > > is well sorted. > > Why does sorting matter? It reduces merge conflicts - a lot. Because > when people add the same import they'll do so at the same line, and > different imports will be spread out rather than all at the bottom. > > -Rob Awesome. Thanks for the breakdown on that. I've updated the imports accordingly and pushed. -- https://code.edge.launchpad.net/~oubiwann/txaws/413738-method-name-cleanup/+merge/10245 Your team txAWS Team is subscribed to branch lp:txaws. ___ Mailing list: https://launchpad.net/~txawsteam Post to : txawsteam@lists.launchpad.net Unsubscribe : https://launchpad.net/~txawsteam More help : https://help.launchpad.net/ListHelp
Re: [Txawsteam] [Merge] lp:~oubiwann/txaws/413738-method-name-cleanup into lp:txaws
On Wed, Aug 19, 2009 at 11:04 PM, Duncan McGreggor wrote: > On Wed, Aug 19, 2009 at 10:46 PM, Robert > Collins wrote: >> Uhm, I'm not sure :) >> >> The only remaining thing was import sorting. I don't think you replied >> to my reply on that. > > Hrm, well I don't know what you're sorting one... can you explain it? Er, sorting *on* :-) d -- https://code.launchpad.net/~oubiwann/txaws/413738-method-name-cleanup/+merge/10245 Your team txAWS Team is subscribed to branch lp:txaws. ___ Mailing list: https://launchpad.net/~txawsteam Post to : txawsteam@lists.launchpad.net Unsubscribe : https://launchpad.net/~txawsteam More help : https://help.launchpad.net/ListHelp
Re: [Txawsteam] [Merge] lp:~oubiwann/txaws/413738-method-name-cleanup into lp:txaws
On Thu, 2009-08-20 at 02:06 +, Duncan McGreggor wrote: > On Wed, Aug 19, 2009 at 10:46 PM, Robert > Collins wrote: > > Uhm, I'm not sure :) > > > > The only remaining thing was import sorting. I don't think you replied > > to my reply on that. > > Hrm, well I don't know what you're sorting one... can you explain it? the module name, and the item. from foo import bar from gam import alpha, beta from gam.quux import zulu is well sorted. Why does sorting matter? It reduces merge conflicts - a lot. Because when people add the same import they'll do so at the same line, and different imports will be spread out rather than all at the bottom. -Rob -- https://code.launchpad.net/~oubiwann/txaws/413738-method-name-cleanup/+merge/10245 Your team txAWS Team is subscribed to branch lp:txaws. ___ Mailing list: https://launchpad.net/~txawsteam Post to : txawsteam@lists.launchpad.net Unsubscribe : https://launchpad.net/~txawsteam More help : https://help.launchpad.net/ListHelp
Re: [Txawsteam] [Merge] lp:~oubiwann/txaws/413738-method-name-cleanup into lp:txaws
On Wed, Aug 19, 2009 at 10:46 PM, Robert Collins wrote: > Uhm, I'm not sure :) > > The only remaining thing was import sorting. I don't think you replied > to my reply on that. Hrm, well I don't know what you're sorting one... can you explain it? d -- https://code.launchpad.net/~oubiwann/txaws/413738-method-name-cleanup/+merge/10245 Your team txAWS Team is subscribed to branch lp:txaws. ___ Mailing list: https://launchpad.net/~txawsteam Post to : txawsteam@lists.launchpad.net Unsubscribe : https://launchpad.net/~txawsteam More help : https://help.launchpad.net/ListHelp
Re: [Txawsteam] [Merge] lp:~oubiwann/txaws/413738-method-name-cleanup into lp:txaws
Uhm, I'm not sure :) The only remaining thing was import sorting. I don't think you replied to my reply on that. -Rob -- https://code.launchpad.net/~oubiwann/txaws/413738-method-name-cleanup/+merge/10245 Your team txAWS Team is subscribed to branch lp:txaws. ___ Mailing list: https://launchpad.net/~txawsteam Post to : txawsteam@lists.launchpad.net Unsubscribe : https://launchpad.net/~txawsteam More help : https://help.launchpad.net/ListHelp
Re: [Txawsteam] [Merge] lp:~oubiwann/txaws/413738-method-name-cleanup into lp:txaws
Hey Rob, you've got your review labeled as "needs fixing" -- is that still the case? Thanks! -- https://code.edge.launchpad.net/~oubiwann/txaws/413738-method-name-cleanup/+merge/10245 Your team txAWS Team is subscribed to branch lp:txaws. ___ Mailing list: https://launchpad.net/~txawsteam Post to : txawsteam@lists.launchpad.net Unsubscribe : https://launchpad.net/~txawsteam More help : https://help.launchpad.net/ListHelp
Re: [Txawsteam] [Merge] lp:~oubiwann/txaws/413738-method-name-cleanup into lp:txaws
On Tue, 2009-08-18 at 17:48 +, Duncan McGreggor wrote: > > [1] > > So PEP-8 states the following: > > 1. standard library imports > 2. related third party imports > 3. local application/library specific imports > > Both lines are local application, and there's no further specification > on sorting those, is there? Or am I missing something? Oh hmm, its a bzr convention. But its very useful, reduces merge conflicts and makes coffee! -Rob -- https://code.launchpad.net/~oubiwann/txaws/413738-method-name-cleanup/+merge/10245 Your team txAWS Team is subscribed to branch lp:txaws. ___ Mailing list: https://launchpad.net/~txawsteam Post to : txawsteam@lists.launchpad.net Unsubscribe : https://launchpad.net/~txawsteam More help : https://help.launchpad.net/ListHelp
Re: [Txawsteam] [Merge] lp:~oubiwann/txaws/413738-method-name-cleanup into lp:txaws
> On Mon, 2009-08-17 at 13:00 +, Duncan McGreggor wrote:
> >
> > +from txaws.util import XML, calculate_md5
> > from txaws.credentials import AWSCredentials
> > -from txaws.util import XML
>
> PEP8 mandates sorting this:
> from txaws.credentials should be the first line.
[1]
So PEP-8 states the following:
1. standard library imports
2. related third party imports
3. local application/library specific imports
Both lines are local application, and there's no further specification on
sorting those, is there? Or am I missing something?
> > +
> > +
> > +NS = '{http://s3.amazonaws.com/doc/2006-03-01/}'
>
> Rather than NS, perhaps name_space.
[2]
Good idea -- done.
> > class S3Request(object):
> > -def __init__(self, verb, bucket=None, objectName=None, data='',
> > -contentType=None, metadata={},
> > rootURI='https://s3.amazonaws.com',
> > -creds=None):
> > +
> > +def __init__(
> > +self, verb, bucket=None, object_name=None, data='',
> > content_type=None,
> > +metadata={}, root_uri='https://s3.amazonaws.com',
> > creds=None):
>
> I believe PEP-8 allows the style of wrapping that was in use before - I
> won't insist either way, but I do fine using as much of the line as
> possible nicer. e.g
>
> def __init__(self, verb, bucket, object_name, data,
> content_type, metadata):
[3]
I don't think that's PEP-8, but I may be misreading the PEP. If you take a look
at the section that starts with "The preferred way of wrapping long lines..."
and the example code below that, they wrap to the point where the parens of the
contained code starts, not a 4-space indent (I was actually only made aware of
this last year).
I compromised, and re-wrapped like this:
def __init__(self, verb, bucket, object_name, data,
content_type, metadata):
> > === modified file 'txaws/storage/test/test_client.py'
> > --- txaws/storage/test/test_client.py 2009-04-26 08:32:36 +
> > +++ txaws/storage/test/test_client.py 2009-08-17 12:49:40 +
> > @@ -4,17 +4,21 @@
> >
> > from twisted.internet.defer import succeed
>
> c,s,t,u - sorting :) on the import lines below.
>
> > +from txaws.util import calculate_md5
> > +from txaws.tests import TXAWSTestCase
> > from txaws.credentials import AWSCredentials
> > -from txaws.tests import TXAWSTestCase
> > -from txaws.storage.client import S3, S3Request, calculateMD5
> > +from txaws.storage.client import S3, S3Request
> > +
[4]
Again, same question about the ordering... can you be more specific about what
part of PEP-8 I'm missing? Thanks!
> > === modified file 'txaws/util.py'
> > --- txaws/util.py 2009-04-27 08:53:11 +
> > +++ txaws/util.py 2009-08-17 12:49:40 +
> > @@ -6,16 +6,23 @@
> >
> > __all__ = ['hmac_sha1', 'iso8601time']
>
> and here too.
>
> > +import time
> > +import hmac
> > +from hashlib import sha1, md5
> > from base64 import b64encode
> > -from hashlib import sha1
> > -import hmac
> > -import time
> > +
>
>
> review needsfixing
[5]
Ditto ;-)
--
https://code.edge.launchpad.net/~oubiwann/txaws/413738-method-name-cleanup/+merge/10245
Your team txAWS Team is subscribed to branch lp:txaws.
___
Mailing list: https://launchpad.net/~txawsteam
Post to : txawsteam@lists.launchpad.net
Unsubscribe : https://launchpad.net/~txawsteam
More help : https://help.launchpad.net/ListHelp
Re: [Txawsteam] [Merge] lp:~oubiwann/txaws/413738-method-name-cleanup into lp:txaws
Review: Needs Fixing
On Mon, 2009-08-17 at 13:00 +, Duncan McGreggor wrote:
>
> +from txaws.util import XML, calculate_md5
> from txaws.credentials import AWSCredentials
> -from txaws.util import XML
PEP8 mandates sorting this:
from txaws.credentials should be the first line.
> +
> +
> +NS = '{http://s3.amazonaws.com/doc/2006-03-01/}'
Rather than NS, perhaps name_space.
> class S3Request(object):
> -def __init__(self, verb, bucket=None, objectName=None, data='',
> -contentType=None, metadata={},
> rootURI='https://s3.amazonaws.com',
> -creds=None):
> +
> +def __init__(
> +self, verb, bucket=None, object_name=None, data='',
> content_type=None,
> +metadata={}, root_uri='https://s3.amazonaws.com',
> creds=None):
I believe PEP-8 allows the style of wrapping that was in use before - I
won't insist either way, but I do fine using as much of the line as
possible nicer. e.g
def __init__(self, verb, bucket, object_name, data,
content_type, metadata):
> === modified file 'txaws/storage/test/test_client.py'
> --- txaws/storage/test/test_client.py 2009-04-26 08:32:36 +
> +++ txaws/storage/test/test_client.py 2009-08-17 12:49:40 +
> @@ -4,17 +4,21 @@
>
> from twisted.internet.defer import succeed
c,s,t,u - sorting :) on the import lines below.
> +from txaws.util import calculate_md5
> +from txaws.tests import TXAWSTestCase
> from txaws.credentials import AWSCredentials
> -from txaws.tests import TXAWSTestCase
> -from txaws.storage.client import S3, S3Request, calculateMD5
> +from txaws.storage.client import S3, S3Request
> +
>
> === modified file 'txaws/util.py'
> --- txaws/util.py 2009-04-27 08:53:11 +
> +++ txaws/util.py 2009-08-17 12:49:40 +
> @@ -6,16 +6,23 @@
>
> __all__ = ['hmac_sha1', 'iso8601time']
and here too.
> +import time
> +import hmac
> +from hashlib import sha1, md5
> from base64 import b64encode
> -from hashlib import sha1
> -import hmac
> -import time
> +
review needsfixing
--
https://code.launchpad.net/~oubiwann/txaws/413738-method-name-cleanup/+merge/10245
Your team txAWS Team is subscribed to branch lp:txaws.
___
Mailing list: https://launchpad.net/~txawsteam
Post to : txawsteam@lists.launchpad.net
Unsubscribe : https://launchpad.net/~txawsteam
More help : https://help.launchpad.net/ListHelp
