- Change SingleLoader class name to Loader. There's no reason to add confusion by making the name more complicated. - Remove all print statements such as: print "Hello", or print "in loader". If you need to log something, use python's logging module. - The following error condition is not dealt with properly: If the used calls Loader.load(path) for a file that has an unknown extension, an exception should be raise. As it is load() will call lookup(), which will simply print "Unknown file type '%s'"%ext. Modules using that function will then fail to recognize the error condition. - Look at all the try-except blocks and deal with error conditions. - Golden rule of error handling: if you catch an exception and all you do is calling pass or print, you have not dealt with the error properly. If you think you did, then you should definitely explain why in a comment so that developers looking at the code don't think that it's a bug. - Document all classes and methods, and all inputs and outputs. - Where are the comments for log() going? That should be documented in the code. [changed]- Why is the description of the DataLoader module in setup.py "Python module for fitting"? - The return object of a load seems to be inconsistent and depend only on the particular reader. This will cause problems since the calling modules will be unable to know what to expect. An interface should be defined. * Functional requirements - Multiple readers might be able to read files with a given extension. (For instance, .txt can be read by the 2-column reader and the 3-column reader.) If the first one raises an exception, the second should be tried. This means that all exceptions raised by a reader should be caught. - All data loaded from a file should be stored in a common class object. - Avoid use of __setitem__ from outside a class. - Bug: Loading blah.my.txt won't work. Is fixed in reflectivity registry.py - Bug: The extension recognition is case sensitive and shouldn't be. - BUG: if a reader raises an exception other than ValueError, the loader doesn't catch it. * Error conditions - Trying to load a file with an unknown extension should raise a RuntimeException. - Trying to load a corrupted file with a know file extension should raise a RuntimeException.