Invalid content #79
Invalid content #79
Conversation
- removed old ValueError (was not being raised and may have been from earlier version - added IndexError, which is raised if a version is not found in the file - this is raised when the content contains non standad .fcsv content (eg. html) - added a test case to test_model_auto.py
There was a problem hiding this comment.
Largely looks good to me, with one criticism: In the case where the regex happens to match something on the first line but the rest of the document isn't a properly formatted csv, this will get through the check you added at line 102 but raise an unhandled csv.Error at line 120 (I think), so I don't think this fully solves the problem. Would like to see a test case for this (pathological) situation and some code to handle it gracefully.
|
Looking at it again, I agree that we can probably have a better check in place, but I also see 3 cases where an error is going to get thrown if it passes the first check:
I think (1) or (3) is most likely to occur in the case it passes that first test, in which case you'll probably be checking the .fcsv manually. We could write a check to properly validate the file, but I'm not sure how we go about implementing that without checking the file line by line (essentially what we are doing already anyways). Any ideas? (In the mean time I am going to mock up a fake .fcsv with a proper header and see what happens) |
|
I think we're already handling all those kinds of errors, I just mean that the csv reader won't know what to do when it starts parsing at line 120 if the file has no CSV-like structure, which will lead to an exception we're not handling. See the second last example here for an example of how to handle that kind of case. |
|
Ahh I see what you mean now. Can discuss some more during the call today! |
|
Changes from Tristan to handling valid header with invalid content looks good! Merging the PR and closing |
Handles previously found issue of a server error in the case of invalid content being uploaded (#71). This PR adds code to check for this by checking if a version is found. Also adds in a new test case for this particular error.
Additionally, removes an old error that is no longer raised!