Skip to content

Conversation

@NoahStapp
Copy link
Contributor

No description provided.

@blink1073 blink1073 requested review from blink1073 and removed request for Jibola September 9, 2024 15:56
@NoahStapp
Copy link
Contributor Author

The tests appear to be failing now due to not closing clients correctly--do we want to wait for that ticket to be done first, or have this PR also skip those warnings for now?

@blink1073
Copy link
Member

have this PR also skip those warnings for now?

I'd say skip them. There is a separate set of failures in synchro though

"pymongo.synchronous.cursor",
"pymongo.synchronous.encryption",
"pymongo.synchronous.mongo_client",
"pymongo.synchronous.database",
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain these changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The type guards added in PYTHON-4590 have issues with the synchro import hacking when we import from pymongo.synchronous . I'll have to undo this change since the top-level pymongo.* modules don't export the private members that Motor makes use of.

Copy link
Member

@ShaneHarvey ShaneHarvey Sep 10, 2024

Choose a reason for hiding this comment

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

Ah this is exactly the case I was worried about here:

I'm paranoid this might break apps that Mock pymongo classes but it should be fine. Anything that's mocking should inherit from our base classes anyway (instead of trying to ducktype).

mongodb/mongo-python-driver#1820 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A very accurate prediction, unfortunately I can't find a solution for this issue in Motor that doesn't rely on also changing PyMongo code.

Copy link
Member

@blink1073 blink1073 Sep 10, 2024

Choose a reason for hiding this comment

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

an option is to get the class names of the full mro and check for the desired name (in PyMongo)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something like type(client).__name__ == "MongoClient"?

Copy link
Member

Choose a reason for hiding this comment

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

But also for subclasses:

>>> class Foo(MongoClient):
...    pass
...
>>> Foo.__mro__
(<class '__main__.Foo'>, <class 'pymongo.synchronous.mongo_client.MongoClient'>, <class 'pymongo.common.BaseObject'>, <class 'typing.Generic'>, <class 'object'>)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'd still run into the same issue with that, right? Since the import paths differ.

Copy link
Member

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

LGTM!

@NoahStapp NoahStapp merged commit 2a9225d into mongodb:master Sep 11, 2024
@NoahStapp NoahStapp deleted the MOTOR-1363 branch September 11, 2024 16:32
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.

3 participants