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

Remove dbref #123

Merged
merged 2 commits into from
Jun 10, 2015
Merged

Remove dbref #123

merged 2 commits into from
Jun 10, 2015

Conversation

IlyaSkriblovsky
Copy link
Contributor

Is there any reason for TxMongo to contain it's own DBRef implementation?

It was probably copied-and-pasted from older version of PyMongo. I suggest to remove it and use PyMongo's bson.dbref.DBRef instead.

But the is a couple of incompatibilities here:

  • TxMongo's DBRef can accept Collection instance as its first argument. PyMongo's DBRef only accept strings;
  • TxMongo's DBRef declared __cmp__ for some mysterious reason. PyMongo's DBRef cannot be meaningfully compared.

I think both these issues are trivial enough to be neglected in favor of more code sharing with PyMongo.

What do you think?

p.s. coverage decrease is expected :)

@fiorix
Copy link
Collaborator

fiorix commented Jun 9, 2015

I wish I could remember why. There's probably something on Git logs.

On Jun 9, 2015, at 4:32 PM, Ilya Skriblovsky notifications@github.com wrote:

Is there any reason for TxMongo to contain it's own DBRef implementation?

It was probably copy-pasted from older version of PyMongo. I suggest to remove it and use PyMongo's bson.dbref.DBRef instead.

But the is a couple of incompatibilities here:

TxMongo's DBRef can accept Collection instance as its first argument. PyMongo's DBRef only accept strings;
TxMongo's DBRef declared cmp for some mysterious reason. PyMongo's DBRef cannot be meaningfully compared.
I think both these issues are trivial enough to be neglected in favor of more code sharing with PyMongo.

What do you think?

p.s. coverage decrease is expected :)

You can view, comment on, or merge this pull request online at:

#123

Commit Summary

Remove TxMongo's DBRef in favor of bson.dbref.DBRef
DBRef testcase removed since we are not going to test PyMongo
File Changes

D tests/test_dbref.py (61)
D txmongo/dbref.py (106)
Patch Links:

https://github.com/twisted/txmongo/pull/123.patch
https://github.com/twisted/txmongo/pull/123.diff

Reply to this email directly or view it on GitHub.

@IlyaSkriblovsky
Copy link
Contributor Author

It seems that TxMongo originally used pymongo's DBRef, but then in d769c58 it was copied it to its own source tree and altered to add ability to pass Collection instance in __init__ instead of collection name string.

PyMongo's DBRef doesn't allow that. I agree that it's a cool feature, but do we need it with the price of incompatibility and code duplication?

@IlyaSkriblovsky
Copy link
Contributor Author

FYI: __cmp__ was replaced by __eq__ in bson.dbref between 2.1.1 and 2.2
mongodb/mongo-python-driver@7474f5c

@psi29a
Copy link
Contributor

psi29a commented Jun 10, 2015

Honestly, we should be following PyMongo's lead here. We don't have the resources of the official client. I'm OK with this.

psi29a added a commit that referenced this pull request Jun 10, 2015
@psi29a psi29a merged commit cbe354c into twisted:master Jun 10, 2015
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

3 participants