-
Notifications
You must be signed in to change notification settings - Fork 1.1k
PYTHON-5653: fix - correct return type annotation for find_one_and_* methods to include None
#2615
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
PYTHON-5653: fix - correct return type annotation for find_one_and_* methods to include None
#2615
Conversation
ShaneHarvey
left a comment
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.
Is this also a bug in the other find_one_and_* methods?
I just checked and it is. I'll update this PR to fix the type hints for those methods shortly. |
064dfaf to
8875123
Compare
find_one_and_update to include Nonefind_one_and_* methods to include None
find_one_and_* methods to include Nonefind_one_and_* methods to include None
caseyclements
left a comment
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've updated the title with a JIRA ticket. Shane is on leave, but this LGTM!
Shane has just begun a sabbatical.
|
Thanks Casey! I see there's some failing checks, but I can't seem to tell what actually failed. Is there anything needed from me or is it something wonky (eg: test infrastructure) on your end? |
|
Failures are unrelated. |
Summary
find_one_and_update's existing type hint makes the false claim that the method returns a_DocumentType. However, the docstring (correctly) tells us that the method "Returns ``None`` if no document matches the filter."The same is true of other
find_one_and_*methods.Changes in this PR
This PR corrects the type hint of the
find_one_and_*methods to indicate that the function may return either_DocumentTypeorNone(rather than just_DocumentType).I've also updated the docstrings for the methods to make this behavior clearer.
Depending on what you consider to be the "surface area" of your library's API, this might be considered a breaking change.
I can't think of any code that would break at runtime (since the PR doesn't change runtime behavior), but it's possible someone has written code like the following. This PR would cause the
assert_typeto fail at "type checking time".Testing Plan
This change isn't changing any runtime behavior, so I'm not sure how I'd test it. I'm open to suggestions for how to test this if you have any.
Screenshots (optional)
N/A
Checklist
Checklist for Author
Checklist for Reviewer {@primary_reviewer}
Focus Areas for Reviewer (optional)