Skip to content

Tidy up the last few Ruff problems in one_shot.py #160

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.

Sign up for GitHub

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

Open
rswarbrick wants to merge 1 commit into
base: master
Choose a base branch
from
+28 −11
Open

Tidy up the last few Ruff problems in one_shot.py #160

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 28 additions & 11 deletions src/dvsim/flow/one_shot.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,9 @@

from abc import abstractmethod
from collections import OrderedDict
from collections.abc import Sequence
from collections.abc import Callable, Mapping, Sequence
from pathlib import Path
from typing import ClassVar

from dvsim.flow.base import FlowCfg
from dvsim.job.data import CompletedJobStatus
Expand All @@ -21,9 +22,25 @@
class OneShotCfg(FlowCfg):
"""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]] = [
Copy link
Copy Markdown
Contributor

@AlexJones0 AlexJones0 Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

*FlowCfg.ignored_wildcards,
"build_mode",
"index",
"test",
]

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
Copy link
Copy Markdown
Contributor

@AlexJones0 AlexJones0 Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: more specific types here?

Suggested change
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]

) -> None:
"""Initialise OneShotCfg object.

Args:
flow_cfg_file: Path to hjson cfg that was loaded.
hjson_data: The parsed hjson from flow_cfg_file.
args: Arguments passed to dvsim
mk_config: A factory method to allow multi-layer builds.

"""
# Options set from command line
self.tool = args.tool
self.verbose = args.verbose
Expand Down Expand Up @@ -76,7 +93,7 @@ def __init__(self, flow_cfg_file, hjson_data, args, mk_config) -> None:

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:
Copy link
Copy Markdown
Contributor

@AlexJones0 AlexJones0 Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def _merge_hjson(self, hjson_data: Mapping) -> None:
def _merge_hjson(self, hjson_data: Mapping[str, Any]) -> None:

# If build_unique is set, then add current timestamp to uniquify it
if self.build_unique:
self.build_dir += "_" + self.timestamp
Expand Down Expand Up @@ -108,7 +125,9 @@ def _expand(self) -> None:

# Purge the output directories. This operates on self.
def _purge(self) -> None:
assert self.scratch_path
if not self.scratch_path:
raise RuntimeError("Scratch path is '', so cannot purge.")

log.info("Purging scratch path %s", self.scratch_path)
rm_path(self.scratch_path)

Expand Down Expand Up @@ -158,7 +177,7 @@ def _gen_results(self, results: Sequence[CompletedJobStatus]) -> None:
"""Generate results for this config."""

@abstractmethod
def gen_results_summary(self):
def gen_results_summary(self) -> str:
"""Gathers the aggregated results from all sub configs."""

def gen_results(self, results: Sequence[CompletedJobStatus]) -> None:
Expand All @@ -183,18 +202,16 @@ def gen_results(self, results: Sequence[CompletedJobStatus]) -> None:
log.info("[scratch_path]: [%s] [%s]", project, item.scratch_path)

# TODO: Implement HTML report using templates
#
# This was previously implemented by rendering the markdown results for the item.

results_dir = Path(self.results_dir)
results_dir.mkdir(exist_ok=True, parents=True)

# (results_dir / self.results_html_name).write_text(
# md_results_to_html(self.results_title, self.css_file, item.results_md)
# )

log.verbose("[report]: [%s] [%s/report.html]", project, item.results_dir)

self.errors_seen |= item.errors_seen

if self.is_primary_cfg:
self.gen_results_summary()
# self.write_results(self.results_html_name, self.results_summary_md)
# TODO: Write a combined HTML report to self.results_html_name
Loading