Code review comment for lp:~oubiwann/txaws/413738-method-name-cleanup

Revision history for this message
Robert Collins (lifeless) wrote :

On Mon, 2009-08-17 at 13:00 +0000, 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 +0000
> +++ txaws/storage/test/test_client.py 2009-08-17 12:49:40 +0000
> @@ -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 +0000
> +++ txaws/util.py 2009-08-17 12:49:40 +0000
> @@ -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

review: Needs Fixing

« Back to merge proposal