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