Adds alternative to use a mongodb backend #202
Conversation
There was a problem hiding this comment.
Looks good to me, although with some few comments. Do you think we need to test this on stage, assuring that none of these changes interfere with the existing functionality using sql as backend?
One of the builds failed on travis, if you want I can look into this. All tests seem to run fine when I run them locally.
| @@ -23,7 +24,12 @@ def dump_json(data, pretty=False): | |||
| @click.pass_context | |||
| def calculate(context): | |||
| """Calculate statistics across samples.""" | |||
| context.obj['db'] = ChanjoDB(uri=context.obj['database']) | |||
| backend = context.obj['backend'] | |||
| if backend == 'mongodb': | |||
There was a problem hiding this comment.
This block of code is repeated several times. Would it be possible to move this to change.cli.base and add the database object to context?
There was a problem hiding this comment.
Sure, could you suggest with a code snippet how you mean?
There was a problem hiding this comment.
'chanjo' was autocorrected to 'change' in comment above it seems...
I'm just thinking something like this in chanjo.cli.base
if backend == 'mongodb':
chanjo_db = ChanjoMongoDB(uri=context.obj['database'])
else:
chanjo_db = ChanjoDB(uri=context.obj['database'])
context.obj['db'] = chanjo_db
and in chanjo.cli.load, calculate, etc.
chanjo_db = context.obj['db']
chanjo/store/mongo/calculate.py
Outdated
| '$avg': '$completeness_20' | ||
| }, | ||
| 'completeness_50': { | ||
| '$avg': '$completeness_20' |
chanjo/store/mongo/calculate.py
Outdated
| '$avg': '$completeness_20' | ||
| }, | ||
| 'completeness_100': { | ||
| '$avg': '$completeness_20' |
There was a problem hiding this comment.
👍
I guess these would need tests so that this type of error would not pass
chanjo/cli/base.py
Outdated
| context.obj['database'] = (database or context.obj.get('database')) | ||
|
|
||
| context.obj['backend'] = 'sql' | ||
| if (context.obj['database'] and 'mongo' in context.obj['database']): |
There was a problem hiding this comment.
Maybe the type of backend is something that should passed in the config, rather than searching the substring 'mongo' in the uri?
There was a problem hiding this comment.
Yes I think you are right
No description provided.