Support str subclasses as dict keys in encode/to_builtins #454
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously we only supported `str` types exactly, or types that would be natively mapped to `str`. We now natively support `str` subclasses as dict keys in `msgspec.convert` as well as all `encode` functions. This is a bit inconsistent with the rest of msgspec, where `str` subclasses aren't natively supported and require an `enc_hook` to support. I think making an exception for dict keys is fine since: - In the long term the `enc_hook` design should be reworked, allowing us to natively handle `str` subclasses everywhere, provided the user doesn't override a serializer for that type. When that's done dicts with str subclasses as keys will "just work" out of the box, I see no reason not to allow that now as well. - The main reason we don't natively support encoding str subclasses (or most subclasses) is that there's no way for a user to override behavior for a natively supported object. Subclassing a str lets the user change how that str is serialized. While I still want to support this for *values*, I find it highly unlikely that a user would want to change how a dict key is serialized. Again, in the long run users should be able to override this behavior in an ergonomic way, but for now I'm making (IMO) the pragmatic choice of supporting the common case without requiring extra effort from users.
jcrist
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously we only supported
strtypes exactly, or types that would be natively mapped tostr. We now natively supportstrsubclasses as dict keys inmsgspec.to_builtinsas well as allencodefunctions.This is a bit inconsistent with the rest of msgspec, where
strsubclasses aren't natively supported and require anenc_hookto support. I think making an exception for dict keys is fine since:In the long term the
enc_hookdesign should be reworked, allowing us to natively handlestrsubclasses everywhere, provided the user doesn't override a serializer for that type. When that's done dicts with str subclasses as keys will "just work" out of the box, I see no reason not to allow that now as well.The main reason we don't natively support encoding str subclasses (or most subclasses) is that there's no way for a user to override behavior for a natively supported object. Subclassing a str lets the user change how that str is serialized. While I still want to support this for values, I find it highly unlikely that a user would want to change how a dict key is serialized. Again, in the long run users should be able to override this behavior in an ergonomic way, but for now I'm making (IMO) the pragmatic choice of supporting the common case without requiring extra effort from users.
Fixes #450.