-
Notifications
You must be signed in to change notification settings - Fork 12
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
fix(actions): use mutable generic container #508
Conversation
Codecov ReportBase: 50.74% // Head: 52.21% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #508 +/- ##
==========================================
+ Coverage 50.74% 52.21% +1.46%
==========================================
Files 124 124
Lines 8207 8161 -46
Branches 1274 1263 -11
==========================================
+ Hits 4165 4261 +96
+ Misses 3882 3725 -157
- Partials 160 175 +15
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
…entation" This reverts commit afb8bad.
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.
Overall stuff still runs
status: StepResult | ||
|
||
def __init_subclass__(cls, **kwargs: tp.Any): | ||
super().__init_subclass__(**kwargs) |
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.
Here is no base class here, so super does not exist
|
||
StepTy_co = tp.TypeVar("StepTy_co", bound=Step, covariant=True) |
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.
You want this still bound on Step
?
Ok, from my side I have two "issues" here.
a) Protocols can only inherit protocols, so not Step
b) I change the call function
b -> I can fix by calling my function a different name
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.
No not necessarily.
This seems like a minimal patch to improve type-safety in utils/actions.py.
Removing the bound requires more changes by all MultiStep descendants:
- Any
- Experiment
- RequireAll
I haven't actually thought about implementing them there yet, so that might be a minimal change as well.
No description provided.