-
Notifications
You must be signed in to change notification settings - Fork 22
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
feanil/unpin pymongo #317
feanil/unpin pymongo #317
Conversation
We only use very simple objects from pymongo in this repo and none of them have changes in the intermediate versions. The only usage of the SON object is to instantiate one and so no code in this repo is going to break by this upgrade. There may be other repos that upstream that might need to pin pymongo based on their usage but it shouldn't be pinned here.
76c1285
to
dfaf635
Compare
@ormsbee @kdmccormick tagging you in case there is some historic context you have for why this pin is here. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #317 +/- ##
=======================================
Coverage 93.96% 93.96%
=======================================
Files 29 29
Lines 2849 2849
Branches 640 640
=======================================
Hits 2677 2677
Misses 145 145
Partials 27 27
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Here's the PR that pinned it: #304
It seems like it was pinned out of an abundance of caution over this breaking change: https://pymongo.readthedocs.io/en/4.4.0/migrate-to-pymongo4.html#son-items-now-returns-dict-items-object
On one hand, these BSON objects are part of the opaque-keys interface, so breaking changes to BSON could be considered breaking changes to opaque-keys.
On the other hand, the breakage is very minor, and also very easy to detect: there'll be a ValueError, TypeError or AttributeError if someone was depending on .items()
to return a list. It's not like some pernicious encoding change that could fly under the radar in downstream packages. So, I think it's right to unpin, and let downstreams worry (or not) about the breaking pymongo change.
We only use very simple objects from pymongo in this repo and none of
them have changes in the intermediate versions. The only usage of the
SON object is to instantiate one and so no code in this repo is going to
break by this upgrade.
There may be other repos that upstream that might need to pin pymongo
based on their usage but it shouldn't be pinned here.
make upgrade