-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[DS][25/n] Narrow extra_state type #21613
Conversation
@@ -31,6 +30,10 @@ | |||
|
|||
T = TypeVar("T") | |||
|
|||
# state objects that store arbitrary data between ticks | |||
# if a need for new types arises, they can be added here, as long as they are serializable | |||
SerializableState = Union[None, AssetSubset, Sequence[ValidAssetSubset]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should assign a name to this union. It's too general of name given what it is.
Can we just canonicalize to 'Optional[Sequence[ValidAssetSubset]]`?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
back to your q. the type of this raises additional questions
Fine with not giving it a name, but I don't want to mess with disallowing a singular AssetSubset at the moment (as we have plenty of already-stored information that is just a single AssetSubset) |
Makes complete sense to not change then given that we are dealing with already persisted data 👍🏻 |
6baf92a
to
e5e6ed8
Compare
2baaba6
to
bf37d9b
Compare
e5e6ed8
to
41e4314
Compare
bf37d9b
to
c3c890f
Compare
41e4314
to
a03b80f
Compare
c3c890f
to
3ccd4be
Compare
Merge activity
|
a03b80f
to
2fb533a
Compare
3ccd4be
to
7b5f20e
Compare
## Summary & Motivation As title -- we only need a subset of the full PackableValue type, and can always widen this later ## How I Tested These Changes
## Summary & Motivation As title -- we only need a subset of the full PackableValue type, and can always widen this later ## How I Tested These Changes
Summary & Motivation
As title -- we only need a subset of the full PackableValue type, and can always widen this later
How I Tested These Changes