-
Notifications
You must be signed in to change notification settings - Fork 4
inserting data to qiita for testing #129
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @antgonza --the only concern I have her is with adding what appears (maybe?) to be real info as test data, particularly in the sql . Are all the qiita studies and metadata that are added here already public in qiita? If I'm reading Qiita right, it seems like at least one isn't, although I didn't check them all. @wasade had raised a concern about this in the past in other repos, so I think we want to avoid repeating it in this one!
Thank you for your review. IMOO this is not a concern as the sample-sheet doesn't contain any actual metadata or identifying information. However, happy to change whatever you think might not be OK, just let me know. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small request for extension to an error message. Thank you for all the test file changes--I am quite aware of what a pain they are!
src/qp_klp/Workflows.py
Outdated
msgs.append("Number of values in sheet that aren't " | ||
"tube-ids in Qiita: %s" % len(results_tid[0])) | ||
rtid = results_tid[0] | ||
msgs.append('Number of sample-names not in Qiita: ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this revised message is providing all the necessary info, which is that not only are these identifiers not found as sample names in qiita, they are also not found as tube ids in qiita. I definitely would prefer that it not be identical in wording to the message at 507. Can you modify?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes!
|
||
|
||
if __name__ == '__main__': | ||
main() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just curious--what necessitates this? I can't recall having to do this with a unit test module before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like to run all tests via nosetests locally, I prefer to just to python [file-to-test], for that to work you need those 2 lines.
No description provided.