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 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