Skip to content

Conversation

adamchainz
Copy link
Contributor

Python 3.0 removed dict.has_key() and the dict.iter*() methods.

fcedc51 removed SON.iteritems() to align with its removal in Python 3. This commit cleans up by removing the remaining compatibility methods: has_key(), iterkeys(), and itervalues().

@adamchainz adamchainz requested a review from a team as a code owner September 18, 2025 13:04
@blink1073 blink1073 self-requested a review September 18, 2025 14:29
@blink1073
Copy link
Member

The encryption failure is because you're missing c0e0554. Can you please pull from master?

@blink1073
Copy link
Member

Merge commits are fine, we squash on merge.

@ShaneHarvey
Copy link
Member

ShaneHarvey commented Sep 18, 2025

Since this is a breaking API change, we can't merge this until we plan a pymongo 5.0 release. What's the motivation here? These methods don't add any maintenance burden so I'm inclined to keep them so that we don't break applications.

@adamchainz
Copy link
Contributor Author

Hi @ShaneHarvey

I emailed @blink1073 , @NoahStapp and @aclark4life as maintainers because I saw they had recent commits, but missed you. I'm working on optimizing my client Rippling's MongoDB-based Django codebase, which uses a private fork of mongoengine, which is backed by pymongo.

This change in particular was a kind of warmup. I would like to make SON inherit from dict, so it can use the C-level builtin methods of dict. I think it's possible because SON currently exists to store keys in insertion order, but that's the default for dict since Python 3.6.

I thought on the way, we could clear out these compatibility methods. I totally understand if you want to make that a major version bump, in which case I can work on making SON inherit from dict in a single, larger PR>

@ShaneHarvey
Copy link
Member

We previously considered making SON inherit from dict but we rejected that approach because the risk of breaking user applications is too high. Instead users are expected to switch from SON to dict themselves. Perhaps we should update mongoengine to replace SON with dict instead? That would have a smaller "blast radius".

@ShaneHarvey
Copy link
Member

I totally understand if you want to make that a major version bump, in which case I can work on making SON inherit from dict in a single, larger PR

We aren't planning on doing a 5.0 release anytime soon so I don't think it's worthwhile to work on this at this time. Next steps would be to consider changing mongoengine to replace SON with dict (MongoEngine/mongoengine#2898), updating our SON docs to discourage new applications from using it, and maybe opening a new ticket to track revisiting SON->dict change in our next major release.

@ShaneHarvey
Copy link
Member

Also if it's possible to optimize SON without any breaking changes that could be worthwhile.

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