-
Notifications
You must be signed in to change notification settings - Fork 1.1k
PYTHON-4590 - Add type guards to async API methods #1820
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to do this? Doesn't the user already get an error if they pass in the wrong type? Could you give some examples?
pymongo/asynchronous/bulk.py
Outdated
"""A proxy for SocketInfo.write_command that handles event publishing.""" | ||
if not isinstance(client, AsyncMongoClient): | ||
raise TypeError( | ||
f"AsyncMongoClient required but {client} is an instance of {type(client)}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this ever be reached? Wouldn't something else fail beforehand?
The idea is that the current behavior looks like this:
This change gives us this:
|
@@ -176,6 +176,7 @@ | |||
if TYPE_CHECKING: | |||
from types import TracebackType | |||
|
|||
from pymongo.asynchronous.mongo_client import AsyncMongoClient |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this is no longer used
pymongo/asynchronous/database.py
Outdated
if not isinstance(name, str): | ||
raise TypeError("name must be an instance of str") | ||
|
||
if not isinstance(client, AsyncMongoClient): | ||
raise TypeError( | ||
f"AsyncMongoClient required but {client} is an instance of {type(client)}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to include both the repr(client) and the type(client)? It could be safer to only add the type to avoid adding topology/hostname info into these errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I think type(client)
makes more sense in this instance.
@@ -231,6 +231,12 @@ def __init__( | |||
) | |||
if not isinstance(name, str): | |||
raise TypeError("name must be an instance of str") | |||
from pymongo.synchronous.database import Database | |||
|
|||
if not isinstance(database, Database): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, we'll have to watch out for bug reports with this.
pymongo/asynchronous/collection.py
Outdated
|
||
if not isinstance(database, AsyncDatabase): | ||
raise TypeError( | ||
f"AsyncCollection requires an AsyncDatabase, {database} is an instance of {type(database)}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should make the same {client}
error message change to all the Database ones as well since Database's repr includes the client.
No description provided.