txawsteam team mailing list archive

[Robert Collins <robertc@xxxxxxxxxxxxxxxxx>]

Wow, code explosion. Uhm, This perhaps would be easier to review in
parts...

Firstly, the module should be txaws.storage.simulator, I think. Or
storage.server.simulator.

Secondly, copyright headers. txaws does a much leaner one;

# Copyright (C) 2009 $PUT_YOUR_NAMES_HERE
# Licenced under the txaws licence available at /LICENSE in the txaws
source.

Please use that - it prevents skew between different modules.

See txaws/ec2/client.py, for instance.


Thirdly, I'm not sure what python versions we're aiming for. I note that
the simulator code depends on 'with', which carries an implicit version
requirement. Please document the version that the simulator supports
both in /README and perhaps the docstring for the simulator.



On Wed, 2009-08-19 at 14:45 +0000, Elliot Murphy wrote:
> === modified file 'README'
> --- README      2009-04-26 08:32:36 +0000
> +++ README      2009-08-19 14:36:56 +0000
> @@ -7,6 +7,10 @@
>  
>  * The epsilon python package (python-epsilon on Debian or similar
> systems)
>  
> +* The S4 test server has a dependency on boto (python-boto) on Debian
> or similar)
> +  This dependency should go away in favor of using txaws
> infrastructure (s4 was
> +  originally developed separately from txaws)

This is a problem :). I'd much rather see code land without having a
boto dependency at any point: boto is rather ugly, and the code will
likely be a lot nicer right from the get-go if we don't have it.


> === added file 'txaws/s4/README'

see above - the location doesn't fit with the txaws source layout. This
is a storage server module (contrast with txaws.storage.client).

Having a README in a python module is odd. The content would be better
put in the __init__.py's docstring, so that pydoctor etc can show it.


> --- txaws/s4/README     1970-01-01 00:00:00 +0000
> +++ txaws/s4/README     2009-08-19 14:36:56 +0000
> @@ -0,0 +1,30 @@
> +S4 - a S3 storage system stub
> +=============================
> +
> +the server comes with some sample scripts so you can see how to use
> it.
> +
> +Using twistd
> +------------
> +
> +to start: ./start-s4.sh
> +to stop: ./stop-s4.sh
> +
> +the sample S4.tac defaults to port 8080. if you want to change that
> you can create your own S4.tac. 

Given that this is a twisted process, it would be nice for the docs to
say 
'to start: twistd -FOO -BAR -BAZ' rather than referring to a shell
script which by its nature won't work on windows.


> 
> === added directory 'txaws/s4/contrib'
> === added file 'txaws/s4/contrib/S3.py'
> --- txaws/s4/contrib/S3.py      1970-01-01 00:00:00 +0000
> +++ txaws/s4/contrib/S3.py      2009-08-19 14:36:56 +0000

....

What is this file for? how is it used? It looks like a lot of
duplication with existing code in txaws.
The different (C) terms means it will need to be mentioned in some way
at the top level portion.
I suspect we need to add an AUTHORS file too.


> === added file 'txaws/s4/s4.py'
> ...+if __name__ == "__main__":
> +    root = Root()
> +    site = server.Site(root)
> +    reactor.listenTCP(8808, site)
> +    reactor.run()

I've skipped most of this file pending the boto dependency being
removed. But I thought I'd mention that this fragment above is highly
redundant with shipping a tac - it shouldn't be needed at all.

> === added file 'txaws/s4/s4_xml.py'
> --- txaws/s4/s4_xml.py  1970-01-01 00:00:00 +0000
> +++ txaws/s4/s4_xml.py  2009-08-19 14:36:56 +0000

Again, I've largely skipped this file - the review is huge and I don't
want to make you wait to start getting it ready for me to have time to
read the entire thing. 

embedding a comment like this in the code makes the code harder to read.
Putting it in some actual documentation somewhere would be better (or
even just reference the AWS spec).
...
> +
> +if __name__ == '__main__':
> +    # pylint: disable-msg=W0403
> +    # pylint: disable-msg=E0611    
> +    from s4 import Bucket
> +    bucket = Bucket("test-bucket")
> +    lbr = ListBucketResult(bucket)
> +    print to_XML(lbr)
> +    print

I really don't like this style of adhoc testing - please write a unit
test and have it perform a string equality, if thats needed. Otherwise
its dead code - it won't be checked regularly, can skew and thats a
problem. Or if its not a problem, we don't need it at all :).


> == added directory 'txaws/s4/testing'

Calling this 'test_support' would make it clearer that its not tests.


> === added directory 'txaws/s4/tests'
> ...        
> +    @defer_to_thread
> +    def test_get(self):
...
This is a bit of a smell - why do we need to use threads in testing this
code?
...
> +def test_suite():
> +    """Used by the rest runner to find the tests in this module"""
> +    return unittest.TestLoader().loadTestsFromName(__name__)
> +
> +if __name__ == "__main__":
> +    unittest.main()

This boilerplate isn't needed:
python -m unittest <module> is precisely equivalent to that. 


> +if __name__ == '__main__':
> +    suite = unittest.TestSuite()
> +    suite.addTest(unittest.makeSuite(S3ConnectionTest))
> +    unittest.TextTestRunner(verbosity=2).run(suite) 

Ditto.

review needsfixing

-- 
https://code.launchpad.net/~statik/txaws/here-have-some-s4/+merge/10388
Your team txAWS Team is subscribed to branch lp:txaws.