-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
test(aci): Opt more of workflow_engine into the mypy stronglist #93671
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #93671 +/- ##
===========================================
+ Coverage 46.31% 88.02% +41.70%
===========================================
Files 10310 10327 +17
Lines 594449 595742 +1293
Branches 23127 23127
===========================================
+ Hits 275320 524383 +249063
+ Misses 318636 70866 -247770
Partials 493 493 |
class ActionGroupStatusAnnotations(TypedDict): | ||
frequency: int | ||
frequency_minutes: timedelta | ||
difference: timedelta | ||
|
||
|
||
if TYPE_CHECKING: | ||
AnnotatedActionGroupStatus = WithAnnotations[ActionGroupStatus, ActionGroupStatusAnnotations] | ||
else: | ||
AnnotatedActionGroupStatus = Any |
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.
we shouldn't be stuffing attributes into random models. please make something which wraps this properly rather than this hack
also note that WithAnnotations
is broken so this will make mypy cache not work properly
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.
Can you point me to an example of what you mean by 'something which wraps this properly'? I don't recall seeing any other examples of annotated things that work with mypy, so when I saw this case, I went with what the django mypy lib recommends for this case.
Not a long term concern for this particular PR, as I have another PR brewing that deletes this code, just for my general education.
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.
oh actually sorry I misread this -- still WithAnnotations
is broken and generally to be avoided. I thought this was the other case I'm chasing down where unrelated attributes are being stuffed into a Group
object (not using django's querying engine)
here's an example of one of those such things I cleaned up a while back: #72367
still to make this type safe-r I would adjust this function to return a separate class with the annotated attributes extracted (such that WithAnnotations
doesn't escape this function via return value and end up in the mypy cache)
something like:
class ActionGroupStatusWithFrequency(NamedTuple):
action_status_group: ActionStatusGroup
frequency: ...
...
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.
ok, rebased away the need for this altogether, but thanks for the info.
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.
Add mostly basic type annotations.
Add mostly basic type annotations.
Add mostly basic type annotations.