Allow type_hooks for substituted objects of the same parent type#219
Open
SnijderC wants to merge 8 commits intokonradhalas:masterfrom
Open
Allow type_hooks for substituted objects of the same parent type#219SnijderC wants to merge 8 commits intokonradhalas:masterfrom
SnijderC wants to merge 8 commits intokonradhalas:masterfrom
Conversation
…tionally returning or falsy values.
Author
|
@konradhalas I appreciate you probably develop this in your free time, don't want to bother you, but any chance you could review this please? :) |
Collaborator
|
@SnijderC Hi there! We will look into open PRs and issues soon, sorry for the delay! |
|
To quote a song: How soon is now? :) |
This file contains hidden or 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
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.
We've had issues with dacite not supporting
type_hooksondatetimeduring testing, whenfreezegunis monkey patching thedatetimeobject to beFakeDatetime, see here.At first it was assumed that the dataclass attribute was monkey patched and therefore the
type_hookslist doesn't have a match for the attribute's type. However this is not the case, as the interpreter would have already interpreted the type annotation before the monkey patch takes place, taking a reference to the original object.It is instead the
type_hooks' key (which for clarity is not a string but instead the data type itself) that is patched. Thus on structuring data into a dataclass, thetype_hooksdict contains aFakeDatetimekey to anisoparsehook.Last bit of context needed before going to the proposed fix: the way type hooks are matched with dataclass's attributes's types in dacite today is by
in, which is like iterating over the keys and doing a value comparison (==, notis). This does however not allow us to have objects to be replaced following the Liskov Principle, whereby we can substitute an object of one type by another so long as they share the same parent (simplified for brevity, sufficient for this context).Coming to the proposal: If we change the way we compare type_hooks with "is a ..." logic instead of "==" logic, we can substitute objects that should provide the same behaviour.
Getting back to the original problem, freezegun. Since Python allows us to redefine the behaviour of objects via meta classes, we can create objects that aren't actual drop-ins but will still declare themselves as a subclass of another type. This is exactly what freezegun is doing here. This object is not a subclass of
datetime.datetimeby inheritance, instead by explicitly overriding the comparison made byissubclass. With the proposed change, this means we can monkey patchdatetime.datetimetoFakeDateTimeand still have a matching type hook in thetype_hooksdict. I imagine this will be helpful for a lot more cases where monkey patching is happening with objects defined in a similar way.