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

feanil/unpin pymongo #317

Merged
merged 2 commits into from
Jun 10, 2024
Merged

feanil/unpin pymongo #317

merged 2 commits into from
Jun 10, 2024

Conversation

feanil
Copy link
Contributor

@feanil feanil commented Jun 7, 2024

  • fix: Unpin Pymongo.
    • 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.

  • chore: Run make upgrade

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.
@feanil feanil requested review from ormsbee, kdmccormick and farhan and removed request for ormsbee, kdmccormick and farhan June 7, 2024 23:23
@feanil
Copy link
Contributor Author

feanil commented Jun 7, 2024

@ormsbee @kdmccormick tagging you in case there is some historic context you have for why this pin is here.

Copy link

codecov bot commented Jun 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.96%. Comparing base (621638d) to head (dfaf635).
Report is 21 commits behind head on master.

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           
Flag Coverage Δ
unittests 93.96% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@kdmccormick kdmccormick left a 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.

@feanil feanil merged commit bf60552 into master Jun 10, 2024
17 checks passed
@feanil feanil deleted the feanil/unpin-pymongo branch June 10, 2024 13:45
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