Construct instance of appropriate class in Event._init #752
Conversation
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
1 task
|
/ok to test eb6890b |
shwina
commented
Jul 11, 2025
| @@ -89,7 +89,7 @@ cdef class Event: | |||
|
|
|||
| @classmethod | |||
| def _init(cls, device_id: int, ctx_handle: Context, options=None): | |||
| cdef Event self = Event.__new__(Event) | |||
| cdef Event self = Event.__new__(cls) | |||
There was a problem hiding this comment.
This ensures that a cls instance is produced here, which could be different from Event if cls is a subclass.
There was a problem hiding this comment.
I think this works! I was trying:
cdef cls self = Event.__new__(cls)and getting compile time errors.
|
carterbox
suggested changes
Jul 11, 2025
kkraus14
reviewed
Jul 11, 2025
| @@ -89,7 +89,7 @@ cdef class Event: | |||
|
|
|||
| @classmethod | |||
| def _init(cls, device_id: int, ctx_handle: Context, options=None): | |||
| cdef Event self = Event.__new__(Event) | |||
| cdef Event self = Event.__new__(cls) | |||
There was a problem hiding this comment.
Can we add a test for a derived class of Event to ensure that we don't accidentally regress on what this enables?
There was a problem hiding this comment.
We can go about this in two ways:
- Write a test that invokes
MyEvent.__new__directly and test that it produces aMyEvent(easy) - Write a test that monkeypatches
EventtoMyEvent, and test that constructing an event viaStreamorDeviceproduces aMyEventrather than anEvent(more complex, but representative of how users can actually use subclasses).
Ultimately, (2) calls down to (1) but in a way that's an implementation detail. Any preferences?
There was a problem hiding this comment.
Lets take the easy path and we can always revisit in the future if we see value in doing so.
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.
Description
Closes #750
closes
Checklist