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