Tidy up the last few Ruff problems in one_shot.py#160
Tidy up the last few Ruff problems in one_shot.py#160rswarbrick wants to merge 1 commit intolowRISC:masterfrom
Conversation
| """Simple one-shot build flow for non-simulation targets like linting, synthesis and FPV.""" | ||
|
|
||
| ignored_wildcards = [*FlowCfg.ignored_wildcards, "build_mode", "index", "test"] | ||
| ignored_wildcards: ClassVar[list[str]] = [ |
There was a problem hiding this comment.
Aside (you don't need to change it in this PR, it just came to mind as I looked at the code):
It needs a more careful examination, but it looks to me like FlowCfg.ignored_wildcards is only used in its _expand stage (with the HJSON __dict__ replacement magic). That is, I think this class attribute is set once and never mutated (even by the HJSON). If that is the case then this should probably be converted to a ClassVar[tuple[str, ...]] to remove the mutability. Either way the ClassVar type is reasonable here.
There was a problem hiding this comment.
I think that's probably a good idea (and, as you say, probably for a different PR)
src/dvsim/flow/one_shot.py
Outdated
|
|
||
| def __init__(self, flow_cfg_file, hjson_data, args, mk_config) -> None: | ||
| def __init__( | ||
| self, flow_cfg_file: str, hjson_data: Mapping, args: object, mk_config: Callable |
There was a problem hiding this comment.
Nit: more specific types here?
| self, flow_cfg_file: str, hjson_data: Mapping, args: object, mk_config: Callable | |
| self, flow_cfg_file: str, hjson_data: Mapping[str, Any], args: argparse.Namespace, mk_config: Callable[[str], FlowCfg] |
There was a problem hiding this comment.
Good point. Done!
src/dvsim/flow/one_shot.py
Outdated
| super().__init__(flow_cfg_file, hjson_data, args, mk_config) | ||
|
|
||
| def _merge_hjson(self, hjson_data) -> None: | ||
| def _merge_hjson(self, hjson_data: Mapping) -> None: |
There was a problem hiding this comment.
| def _merge_hjson(self, hjson_data: Mapping) -> None: | |
| def _merge_hjson(self, hjson_data: Mapping[str, Any]) -> None: |
There was a problem hiding this comment.
Sounds good. Done!
The biggest change is renaming _gen_results to gen_results_for_cfg. The motivation is that we call it on subconfig items, so it shouldn't really have a private name. I've also made its name a bit more differant from the existing gen_results function. Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>
rswarbrick
force-pushed
the
one-shot-ruff-tweaks
branch
from
e511810 to
5b08ad7
Compare
|
@AlexJones0: Thanks for the review. I've addressed things, but it was a bit more typing than I expected. Accessing the function properly means that Ruff (correctly) points out that we probably shouldn't be calling a private function. All fixed now though, I think. |
2 participants
No description provided.