txawsteam team mailing list archive

[Elliot Murphy <elliot@xxxxxxxxxxxxx>]

Thanks a lot for the quick review. The code is very much in the state  
it was being used internally, and I think your comments all make sense  
and will improve the code. I differ on the license header thing - I  
explicitly chose not to copy the existing indirect way of specifying  
the license. You'd need to go back to the copyright holder to change  
the license anyway, so specifying the license that way is not a good  
idea IMO.

Just to set expectations, I don't expect to have time to remove the  
boto dependency or work on the more involved changes requested before  
Karmic ships. Duncan asked about the code and I got it published in  
the spirit of jfdi; now that it is free to the world in this branch  
I'm back to more pressing Karmic related hacking. I think it's  
entirely reasonable for you to want the boto dependency dropped before  
it's merged, I just want to be up front and explain that this branch  
will probably sit for a couple of months before lucio or I or  
Christian will be able to give it that level of attention. I'd  
actually like to kill the whole contrib directory too.
-- 
Elliot Murphy

On Aug 19, 2009, at 9:45 PM, Robert Collins  
<robertc@xxxxxxxxxxxxxxxxx> wrote:

> 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
> You are the owner of lp:~statik/txaws/here-have-some-s4.
>
https://code.launchpad.net/~statik/txaws/here-have-some-s4/+merge/10388
Your team txAWS Team is subscribed to branch lp:txaws.