Skip to content
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

mongodb additions to add / complete all methods #27

Merged
merged 7 commits into from Jul 23, 2019
Merged

mongodb additions to add / complete all methods #27

merged 7 commits into from Jul 23, 2019

Conversation

rfhb
Copy link
Collaborator

@rfhb rfhb commented Jul 2, 2019

Hi, this is a PR for the mongodb implementation in the package. It completes the set of methods for src_mongo(). Also, it extends already existing functions to plausible and important use cases, e.g. for selectively deleting and updating documents. All changes respect the principles I am aware of for the nodbi package (e.g. using data frames as input, harmonisation of outputs and inputs across connectors). I wish to use the nodbi package in my project ctrdata, where in a local branch src_mongo() and src_sqlite() (PR #25) already work interchangeably. Many thanks for reviewing and considering!

@sckott sckott added this to the v0.3 milestone Jul 2, 2019
@sckott
Copy link
Contributor

sckott commented Jul 2, 2019

thanks - this PR needs some conflict fixing - then i'll take a look

@rfhb
Copy link
Collaborator Author

rfhb commented Jul 3, 2019

Thanks, conflicts are now resolved, were related to changes on another branch that git could not auto-merge.

@sckott
Copy link
Contributor

sckott commented Jul 17, 2019

thanks, having a look

@sckott
Copy link
Contributor

sckott commented Jul 17, 2019

Sorry for delay in looking at this, was on vacation.

Looks okay to me, but I'll see if @jeroen can have a look as he is much more familiar than I with mongodb.

@sckott
Copy link
Contributor

sckott commented Jul 19, 2019

Looks good to me. Some comments:

The create eg no longer works with this PR

src <- src_mongo()
docdb_create(src, key = "mtcars", value = mtcars)
#> Parameter 'key' is different from parameter 'collection', was given as test in src_mongo().

I realize it's intended behavior, but should change the example to have a working eg.

Actually, all the mongo examples and tests have this problem where the src_mongo has to have a matching collection name to the key name used in the fxn calls.

Maybe i'm not understanding this change - seems like with this PR you can only have one collection and only one key in that collection of the same name. Is that correct? If so, what's the reason for that?

@rfhb
Copy link
Collaborator Author

rfhb commented Jul 20, 2019

Thank you @sckott - fixed the warning in the example of the src_mongo() documentation with commit
9159ed1.

Yes, a mongo connection (mongolite::mongo()) can only be opened for one specific collection. The warnings are intended to be helpful in particular for users who may not realise this.

@rfhb
Copy link
Collaborator Author

rfhb commented Jul 21, 2019

Hi - I have now updated all src_mongo() calls and docdb_*() tests for mongo with commits ee96177 436bcd9 e77d8f9 to specify the collection name, thus avoiding warnings when key is different from the collection.

@sckott
Copy link
Contributor

sckott commented Jul 22, 2019

thanks - I'll have a look

Copy link
Contributor

@sckott sckott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please make sure egs work

R/delete.R Show resolved Hide resolved
R/query.R Show resolved Hide resolved
@sckott sckott merged commit 91df68a into ropensci:master Jul 23, 2019
@rfhb rfhb deleted the mongodb-addition branch July 25, 2019 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants