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

GridFS index fixed. codec_options argument for Database.command. #190

Merged
merged 1 commit into from
Oct 12, 2016

Conversation

IlyaSkriblovsky
Copy link
Contributor

It is funny and sad at the same time, but GridFS were creating index with a typo in field name and nobody noticed that :)

Also, ensure_index removed from get_last_version because ASC+ASC index, created in __init__ is sufficient.

For the means of testing indexes creation, I've added codec_options argument to Database.command(). It can be used to make command() to return response as SON instead of plain dict.

@coveralls
Copy link

coveralls commented Oct 4, 2016

Coverage Status

Coverage increased (+0.005%) to 95.587% when pulling 484c47a on IlyaSkriblovsky:fix-gridfs-index into 6225f42 on twisted:master.

@psi29a
Copy link
Contributor

psi29a commented Oct 10, 2016

We now have conflicts... to be expected now with less inline callbacks.

@IlyaSkriblovsky
Copy link
Contributor Author

IlyaSkriblovsky commented Oct 10, 2016

Rebased and solved conflicts.

I've noticed also that adding codec_options to Database.command was unnecessary since it passes all its **kwargs to find_one(), so I just can use as_class=SON. But as_class is deprecated in PyMongo for a long time and codec_options just reflects PyMongo's API, so I suggest to leave it.

P.S.
(Btw, this relates to the discussion of hidden **kwargs in another PR ;) )

P.P.S.
More on **kwargs. Notice how I had to do command() call here: https://github.com/twisted/txmongo/blob/master/txmongo/collection.py#L193
I can't just write command("listCollections", filter={"name": self.name}) as I would like to do because command() uses kwargs twice: it adds them to command payload and also passes them to find_one(). So "filter" kwarg not only adds up to DB command, but also messes it up on lower level. I hope we will eventually clean and tidy all this kwargs magic :)

@coveralls
Copy link

coveralls commented Oct 10, 2016

Coverage Status

Coverage increased (+0.005%) to 95.592% when pulling 6de2064 on IlyaSkriblovsky:fix-gridfs-index into d279627 on twisted:master.

2 similar comments
@coveralls
Copy link

coveralls commented Oct 10, 2016

Coverage Status

Coverage increased (+0.005%) to 95.592% when pulling 6de2064 on IlyaSkriblovsky:fix-gridfs-index into d279627 on twisted:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.005%) to 95.592% when pulling 6de2064 on IlyaSkriblovsky:fix-gridfs-index into d279627 on twisted:master.

@IlyaSkriblovsky
Copy link
Contributor Author

@psi29a how do you feel you about this PR now?

@psi29a
Copy link
Contributor

psi29a commented Oct 12, 2016

LGTM, merging.

@psi29a
Copy link
Contributor

psi29a commented Oct 12, 2016

Agreed about the kwarg magic, it's dark band-aid stuff that we should move away from. I formally apologize for adding to it. ;)

@psi29a psi29a merged commit cdfd693 into twisted:master Oct 12, 2016
@IlyaSkriblovsky IlyaSkriblovsky deleted the fix-gridfs-index branch October 13, 2016 06:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants