ropensci / nodbi

Document DBI connector for R
https://docs.ropensci.org/nodbi
Other
76 stars 6 forks source link

mongodb additions to add / complete all methods #27

Closed rfhb closed 5 years ago

rfhb commented 5 years ago

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 commented 5 years ago

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

rfhb commented 5 years ago

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

sckott commented 5 years ago

thanks, having a look

sckott commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

Hi - I have now updated all srcmongo() 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 commented 5 years ago

thanks - I'll have a look