refactor: [typing] add typing to the Scheduler #32
Conversation
machshev
requested review from
andreaskurth,
rswarbrick and
hcallahan-lowrisc
| item._create_deploy_objects() | ||
|
|
||
| def deploy_objects(self): | ||
| def deploy_objects(self) -> Mapping[Deploy, str]: |
There was a problem hiding this comment.
Definitely not for this PR (but I was reading this file...) I wonder whether this function name needs to be changed to something like get_deploy_objects or maybe whether it should be turned into a property?
At the moment, it's a bit sad that "deploy" is a verb as well as an adjective.
There was a problem hiding this comment.
Yes makes sense, I'd like to eventually refactor the whole thing with tests.
src/dvsim/scheduler.py
Outdated
| msg = f"Scheduler does not contain target {curr_target}" | ||
| raise KeyError(msg) |
There was a problem hiding this comment.
No big deal, but why not stick the string literal directly into the KeyError call?
There was a problem hiding this comment.
Because of the lint rule that complains about having f-string directly in an Exception. I've disabled the one that prevents string literals, so if it wasn't a f-string then it would not complain about it.
I don't have a strong opinion either way really. It does slightly clean up the stack trace to have the intermediate variable. But makes the code base slightly more messy.
machshev
force-pushed
the
typing
branch
5 times, most recently
from
0217eef to
d6f3d8a
Compare
Types added to make it clearer what is going on with the job scheduling. This means the pyright/ruff LSP have more information to be able to jump to definitions and show documentation. Signed-off-by: James McCorrie <[email protected]>
Types added to make it clearer what is going on with the job scheduling. This means the pyright/ruff LSP have more information to be able to jump to definitions and show documentation.