Ready for MERGE: Add command to import preprocessed data #122
Conversation
|
@teravest I'm working on this branch, I've already solved the PR. |
|
I meant the merge conflict |
|
@josenavas you are a collaborator on my branch now. |
teravest
changed the title
|
I think this is ready for review and merge! thanks for the help @josenavas |
| @@ -50,6 +50,23 @@ def insert_study_to_db(owner, title, info): | |||
|
|
|||
|
|
|||
| @qiita_db.command() | |||
| @click.option('--study_id', help="Study id associated with data") | |||
| @click.option('--params_table', help="Name of the paramaters table for the" | |||
There was a problem hiding this comment.
missing a space after the.
|
👍 |
| @click.option('--submitted_to_insdc', is_flag=True, | ||
| help="If provided, the preprocessed data have been submitted" | ||
| " to insdc") | ||
| def insert_preprocessed_data(study_id, filedir, filetype, params_table, |
There was a problem hiding this comment.
similarly, this function starts with insert_, while the function it calls begins with import_. I like the convention where the click function is load_x and then the function that it calls (which resides in commands.py) would be called load_x_cmd
|
A few comments. Thanks @teravest and @josenavas! |
|
Also, new merge conflict. |
|
I think it addressed all the comments but I would like to talk to @adamrp before merging... |
|
@teravest I'll be back soon, this meeting should be over soon |
|
I think this is ready for final review |
|
Looks good to me. 👍 |
| @@ -9,6 +9,8 @@ | |||
| from dateutil.parser import parse | |||
| import pandas as pd | |||
| from functools import partial | |||
| from os import listdir | |||
| from os.path import join | |||
There was a problem hiding this comment.
I think during conflict resolution you moved these imports to the wrong place. pandas and dateutil are third-party imports, so they should come after the functools, os, and os.path imports, separated by a blank line, then another blank line, then the imports from qiita
There was a problem hiding this comment.
|
Very minor comments, and it looks great! Thanks again |
|
👍 once imports cleaned up |
|
failures unrelated. |
Ready for MERGE: Add command to import preprocessed data
Tests will not pass until @josenavas issues a new pull request.