add type annotations and docstrings to devlib #715
Conversation
|
Hi, If this requires changing the minimum supported Python version that means we need to perform a new release of both devlib and the WA project on github and PyPI before we can merge this PR. We should also check with our known users what version of Python they are currently using to ensure we provide sufficient time for any users to migrate if required before dropping support for < 3.10. |
Hi Marc, My changes dont exactly need the 3.10 version. I have modified now, to avoid the explicit dependency on the newer language features, for ex - changing imports of Literal, Protocol and TypedDict from typing_extensions instead of from typing. Still i think it is something we should pursue and move up to 3.10 in near future, but that effort can be taken up independent of this PR. Kindly review my changes and do the needful. Thanks and Regards, |
arjpur01
force-pushed
the
master
branch
4 times, most recently
from
b42c7b4 to
d2f5bb8
Compare
| from typing import Dict, Set, List, Union, Any | ||
| from typing import (Dict, Set, List, Union, Any, | ||
| Tuple, cast, Callable, Optional, | ||
| Pattern, Generator, Match) |
There was a problem hiding this comment.
When I try to exercise this code I',m getting the following error:
Could not load devlib.module.cgroups2: Too few arguments for typing.Generator; actual 1, expected 3
There was a problem hiding this comment.
fixed the error. Generator with single args added with [arg1, Any, None] to avoid the error.
Could you let me know how to exercise this code path?
There was a problem hiding this comment.
Had a look and typing.Generator is actually deprecated since 3.9, collections.abc.Generator should be used.
Also the correct signature here would be Generator[arg1, None, None]. Otherwise, that means the user could send values of any type to the generator and expect the current behavior. That in turn means we can never make use of the sent value without breaking backward compat. If None is used, this means we could widen the allowed types for the sent value in the future, and only preserve the current behavior when None is sent
|
Thanks Marc, I will look into the above and make the changes. meanwhile, please continue to review the code. |
Most of the files are covered, but some of the instruments and unused platforms are not augmented
| self.logger = logging.getLogger(self.__class__.__name__) | ||
|
|
||
| def __enter__(self): | ||
| """ | ||
| Enter the context for this runner. | ||
| :return: This runner instance. |
There was a problem hiding this comment.
Shouldn't this just be encoded as annotations ? i.e.:
def __enter__(self) -> TargetRunner:
The description is not bringing any useful information.
| return self | ||
|
|
||
| def __exit__(self, *_): | ||
| """ | ||
| Exit the context for this runner. |
There was a problem hiding this comment.
Ditto, this comment is not useful
| super().__init__(target=target) | ||
|
|
||
| self.boot_timeout = boot_timeout | ||
| self.boot_timeout: int = boot_timeout |
There was a problem hiding this comment.
is the annotation needed ? I'd expect mypy to infer it from boot_timeout type, which is annotated in the signature.
| pass | ||
|
|
||
|
|
||
| QEMUTargetUserSettings = TypedDict("QEMUTargetUserSettings", { |
There was a problem hiding this comment.
There seems to be a class-based way of creating such a TypedDict that avoids repeating the name:
https://typing.python.org/en/latest/spec/typeddict.html#class-based-syntax
| 'enable_kvm': NotRequired[bool], | ||
| }) | ||
|
|
||
| QEMUTargetRunnerSettings = TypedDict("QEMUTargetRunnerSettings", { |
| KernelShark is a graphical front-end tool for visualizing trace data collected by trace-cmd. | ||
| It allows users to view and analyze kernel tracing data in a more intuitive and interactive way. |
There was a problem hiding this comment.
"Open the trace in KernelShark." would do, the rest belongs to KernelShark's own documentation or marketing material.
| """ | ||
| Remove the trace.dat file from the target, cleaning up after data collection. | ||
| """ | ||
| self.target.remove(self.target.path.join(self.target.working_directory, OUTPUT_TRACE_FILE) if self.target.path else '') |
There was a problem hiding this comment.
I never paid attention to this function, what it does is not great. If there are 2 instances of FtraceCollector alive at the same time, the first one may end up removing a file collected by the 2nd one. It's clearly not the only affected function since everything using OUTPUT_TRACE_FILE would have the same problem. It would make more sense for the FtraceCollector to use instance-specific temp folder
| def mark_start(self): | ||
| def mark_start(self) -> None: | ||
| """ | ||
| Write a start marker into the ftrace marker file. |
There was a problem hiding this comment.
s/ftrace marker file/trace_marker file in tracefs/
| def mark_stop(self): | ||
| def mark_stop(self) -> None: | ||
| """ | ||
| Write a stop marker into the ftrace marker file. |
| self.target.write_value(self.marker_file, TRACE_MARKER_STOP, verify=False) | ||
|
|
||
|
|
||
| def _build_trace_events(events): | ||
| event_string = ' '.join(['-e {}'.format(e) for e in events]) | ||
| def _build_trace_events(events: List[str]) -> str: |
There was a problem hiding this comment.
List[str] is restrictive, it should work with any iterable yielding str
Most of the files are covered, but some of the
instruments and unused platforms are not augmented Minimum Python version required change from 3.7 to 3.10 in order to support some of the annotation features