refactor: Remove outdated (and unused) default repo_top argument#218
Conversation
| """Parse an hjson file at the given path and return it as a dict.""" | ||
| return hjson.load(Path(filename).open()) |
There was a problem hiding this comment.
This was a problem in the original code, but we should probably fix it while we're here.
Reference counting probably means that this issue will not be a problem, but we should be explicitly closing the file handle after opening it rather than leaving it open.
A better approach would be:
| """Parse an hjson file at the given path and return it as a dict.""" | |
| return hjson.load(Path(filename).open()) | |
| """Parse an hjson file at the given path and return it as a dict.""" | |
| with Path(filename).open("r", encoding="utf-8") as f: | |
| return hjson.load(f) |
Or maybe even better as:
| """Parse an hjson file at the given path and return it as a dict.""" | |
| return hjson.load(Path(filename).open()) | |
| """Parse an hjson file at the given path and return it as a dict.""" | |
| return hjson.loads(Path(filename).read_text(encoding="utf-8")) |
(to let Pathlib handle the read file mode for us).
There was a problem hiding this comment.
Thanks, that makes sense (and the scope of the file object hadn't occurred to me). I guess the former solution might be nice if we consumed the file as a stream with some sort of reducing operation. But that's not what's going on here, so the second is definitely better (another thing that I understood for the first time this morning...)
| name is an optional argument indicating the name of the testplan / DUT. | ||
| It overrides the name set in the testplan HJson. | ||
| Args: | ||
| filename: Describes the HJson file that captures the testplan. This |
There was a problem hiding this comment.
| filename: Describes the HJson file that captures the testplan. This | |
| filename: Describes the Hjson file that captures the testplan. This |
(a couple more times in these changed docs - I appreciate it was in the original but it'd be nice to be consistent).
There was a problem hiding this comment.
I agree. I've added a staging commit that fixes this.
| is a string, rather than a Path object, because it may be | ||
| suffixed with tags separated with a colon delimiter to | ||
| filter the testpoints. |
There was a problem hiding this comment.
The explanation is great, but given that we later do
filename = Path(split[0])
tags = set(split[1:])I'd suggest the name isn't great anyway and we should rename it. Perhaps something like:
tagged_filenamefilename_with_tagsfile_spec/testplan_file_spec
etc. etc. - take your pick.
There was a problem hiding this comment.
Good point! I'll go for the first.
|
Damn! Wrong project's commit naming convention! Take two... |
Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>
|
And now for plurals. Take three... |
This code was based on dvsim living inside the OpenTitan repository. Fortunately, it is never used: we always pass a third argument to _parse_testplan. Also, fail more understandably if there is an hjson parse error or an invalid filename passed to _parse_hjson. Finally, simplify the Testplan constructor so that it always expects a name for its testplan (rather than exiting the program silently if there isn't one). We also change that argument to explicitly have type str (as opposed to Path) and explain why: the string can have suffix values that filter the set of tests. Signed-off-by: Rupert Swarbrick <rswarbrick@lowrisc.org>
2 participants
This code was based on dvsim living inside the OpenTitan repository. Fortunately, it is never used: we always pass a third argument to _parse_testplan.
Also, fail more understandably if there is an hjson parse error or an invalid filename passed to _parse_hjson.
Finally, simplify the Testplan constructor so that it always expects a name for its testplan (rather than exiting the program silently if there isn't one).
We also change that argument to explicitly have type str (as opposed to Path) and explain why: the string can have suffix values that filter the set of tests.