Recipe modifications #47
Conversation
wolfv
force-pushed
the
recipe-modifications
branch
2 times, most recently
from
4ae9571
to
b03b9eb
Compare
wolfv
force-pushed
the
recipe-modifications
branch
from
b03b9eb
to
9677830
Compare
| return f.getvalue() | ||
|
|
||
|
|
||
| class CouldNotUpdateVersionError(Exception): |
There was a problem hiding this comment.
wdyt about moving errors in separate modules?
| yield source | ||
|
|
||
|
|
||
| def update_hash(source: dict[str, Any], url: str, hash_type: Hash | None) -> None: |
There was a problem hiding this comment.
we can try to annotate it as a typeddict total = False where we expect sha256 and md5 key to be present
| super().__init__(self.message) | ||
|
|
||
|
|
||
| class Hash: |
There was a problem hiding this comment.
maybe let's move it in separate module? like hash.py for example?
| return f"{self.hash_type}: {self.hash_value}" | ||
|
|
||
|
|
||
| def has_jinja_version(url: str) -> bool: |
There was a problem hiding this comment.
I think this also can be moved in utils.py as it seems something useful potentially to have for other modules
|
A more high level question: |
| with file.open("r") as f: | ||
| data = yaml.load(f) |
There was a problem hiding this comment.
Doesnt it make more sense to leave the loading of the yaml to the caller? I can imagine that if we have more modification functions they will all require loading the yaml.
There was a problem hiding this comment.
yes, and no - we do want to minimally change the YAML so I think that should all be handled by us (leave comments in tact, order, formatting, ...).
| with file.open("r") as f: | ||
| data = yaml.load(f) |
There was a problem hiding this comment.
Same here, maybe it would make sense to have a variant that takes the loaded dictionary as input.
|
You should also add proper docstrings to all public functions. Especially explaining their return value and exceptions they can raise. |
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| yaml = YAML() |
There was a problem hiding this comment.
wdyt about extracting YAML loader configuration in one place so it can be used by other loaders?
There was a problem hiding this comment.
yes, I also want to get rid of any PyYAML and just use ruamel.YAML everywhere.
There was a problem hiding this comment.
In fact, that's the next PR that I wanted to send, so let's maybe wait until then?
No description provided.