-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Metrics use json helper #20935
Metrics use json helper #20935
Conversation
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.
Looks sane to me, thanks! But leaving the final review to @allisonkarlitskaya
@@ -21,10 +21,10 @@ | |||
import sys | |||
import time | |||
from collections import defaultdict | |||
from typing import Dict, List, NamedTuple, Optional, Set, Tuple, Union | |||
from typing import Dict, List, NamedTuple, Optional, Set, Tuple, Type, Union |
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've generally grown to prefer using the built-in versions of the types along with quoting...
So, not:
from typing import Dict, Optional, Tuple, Type
x: Optional[Dict[str, Tuple[Type[Sampler], SampleDescription]]]
but
x: 'dict[str, tuple[type[Sampler], SampleDescription]] | None'
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.
Yeah but I mean the code is already there :)
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.
Let's call it a follow-up :)
eaf8973
into
cockpit-project:main
The same should be done for
metrics
, useget_objv
but this was a drive by fix :)FYI: Type is Python 3.9 only, are we that modern already? 😅