Check for non-utf8 characters. Fix #1197 #1198
Conversation
qiita_db/metadata_template/util.py
Outdated
There was a problem hiding this comment.
Can you rise one of the QiitaDB errors?
There was a problem hiding this comment.
And update the documentation of the method to reflect this change?
There was a problem hiding this comment.
None of the QiitaDB errors make much sense here though. Which would you suggest?
There was a problem hiding this comment.
Just use QiitaDBError, at least we know that the errors is generated by us.
|
Just a small comment @squirrelo |
|
Code looks good, could you add a test? |
|
Oh, yup, good call. |
qiita_db/metadata_template/util.py
Outdated
There was a problem hiding this comment.
suggest using enumerate(holdfile, 1) given the use of the index below. Same on the nested forloop
There was a problem hiding this comment.
Nice, didn't know about that. thanks.
|
All comments taken care of. Ready for another round |
|
👍 |
|
Hold on, checking something |
|
Had to revert to ValueError because raising the QiitaDBError caused a 500 error on the interface side. |
|
Is the QiitaDBError not catched on the interface? If that's the case, I would suggest catching it rather than rely on ValueError (I know, more work, but core more robust...) |
|
Done |
|
👍 @wasade can you review this again? |
Check for non-utf8 characters. Fix #1197
|
Yargh, just noticed this PR is missing tests .... 😭 |
|
It does: qiita_db/metadata_template/test/test_util.py |
|
I entirely missed that. 👀 |
6 participants
Does what it says on the tin. Also raises meaningful error with location of cells that have non-utf8 characters.