Move prerun functions into Runtime class #16
Conversation
5b7ce97 to
fc913b5
Compare
Fixed bug found while testing molecule adoption of ansible-compat, mostly related to testing.
| def __init__( | ||
| self, | ||
| config_dump: Optional[str] = None, | ||
| data: Optional[Dict[str, object]] = None, |
| def __init__(self, config_dump: Optional[str] = None) -> None: | ||
| def __init__( | ||
| self, | ||
| config_dump: Optional[str] = None, |
There was a problem hiding this comment.
Multiple ways of initializing the struct in the same initializer looks weird and mixes up multiple unrelated abstractions. It's best to do such things in custom classmethod initializers.
Also, I'd avoid any I/O in the default initializer for a similar reason. The main one should just accept the data and assign it internally.
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.STDOUT, | ||
| ) | ||
| if run.returncode != 0: |
There was a problem hiding this comment.
I'd not use this C-style obligatory rc check idiom. It's unpythonic and error-prone because it adds extra cognitive load on the users who have to always remember to do this check.
It's better to have check=True because this way (1) it forces you to do try/except and you can't just forget to handle an error + (2) you could explicitly chain the exception cause with raise X from Y which greatly improves the DX, specifically the respectability of tracebacks.
| "--roles-path", | ||
| f"{self.cache_dir}/roles", |
There was a problem hiding this comment.
It's usually best to couple options and their values on the same line because otherwise, they look standalone which adds more cognitive complexity for the reader to figure out which ones are related and separate those that aren't.
|
|
||
| if not offline: | ||
| self.install_requirements("requirements.yml") | ||
| for req in pathlib.Path(".").glob("molecule/*/requirements.yml"): |
There was a problem hiding this comment.
Looks like there's an abstraction leak here: compat shouldn't "know" about molecule or lint.
No description provided.