Validate uploaded files #1974
Validate uploaded files #1974
Conversation
|
Yay!! Can I have a review in here? |
| while redis_info['status_msg'] == 'Running': | ||
| sleep(0.05) | ||
| redis_info = loads(r_client.get(job_id)) | ||
| sleep(0.05) |
There was a problem hiding this comment.
Is it just me, or is this pattern being copied and pasted?
| redis_info = loads(r_client.get(job_id)) | ||
| while redis_info['status_msg'] == 'Running': | ||
| sleep(0.05) | ||
| redis_info = loads(r_client.get(job_id)) | ||
|
|
There was a problem hiding this comment.
It is but not sure what the solutions could be as the code is duplicated in two different tests files, do you have an idea of how to solve it?
There was a problem hiding this comment.
I'd build up a library of testing functions in a separate python script to store these sorts of functions.
But its definitely not blocking - if you guys think this is a good idea, I guess this can be raised as a issue to refactor later.
| redis_info = loads(r_client.get(job_id)) | ||
| while redis_info['status_msg'] == 'Running': | ||
| sleep(0.05) | ||
| redis_info = loads(r_client.get(job_id)) |
|
@ElDeveloper @antgonza available for a quick review/merge? |
|
@josenavas, I'm reviewing this right now. |
There was a problem hiding this comment.
@josenavas, this looks good, I have some minor comments. Let me know what you think and we can merge right away since tests are passing already.
| job = ProcessingJob.create( | ||
| User(user_id), | ||
| Parameters.load(command, values_dict={ | ||
| 'template': prep_template_id, |
There was a problem hiding this comment.
Super minor but I think this could be prep.id instead of needing this new variable, right?
There was a problem hiding this comment.
prep_template_id is a parameter to this function, so there is no new variable created.
| from qiita_db.processing_job import ProcessingJob | ||
|
|
||
|
|
||
| def wait_for_prep_information_job(prep_id, raise_if_none=True): |
There was a problem hiding this comment.
Any reason why this function is not documented?
There was a problem hiding this comment.
Oops I forgot! Thanks!
| res = r_client.get('prep_template_%d' % prep_id) | ||
|
|
||
| if raise_if_none: | ||
| assert res is not None, "unexpectedly None" |
There was a problem hiding this comment.
This may be overcautious, but what about raising an actual AssertionError, when we use assert we run the risk that if python is being executed in an "optimized" mode, these statements will be ignored. I suggest this be changed to:
if raise_if_none and res is None
raise AssertionError('Unexpectedly None')There was a problem hiding this comment.
This is actually a really good point - changed!
| from qiita_db.processing_job import ProcessingJob | ||
|
|
||
|
|
||
| def wait_for_prep_information_job(prep_id, raise_if_none=True): |
There was a problem hiding this comment.
Oops I forgot! Thanks!
| res = r_client.get('prep_template_%d' % prep_id) | ||
|
|
||
| if raise_if_none: | ||
| assert res is not None, "unexpectedly None" |
There was a problem hiding this comment.
This is actually a really good point - changed!
| job = ProcessingJob.create( | ||
| User(user_id), | ||
| Parameters.load(command, values_dict={ | ||
| 'template': prep_template_id, |
There was a problem hiding this comment.
prep_template_id is a parameter to this function, so there is no new variable created.
|
Thanks @josenavas! |
|
@josenavas, GitHub seems to be down from UCSD campus :( On (Oct-21-16|10:20), Jose Navas wrote:
|
Fixes #1085