refactor: rewrite configuration handler #163
Conversation
scastlara
force-pushed
the
config-rewrite
branch
3 times, most recently
from
65ea856 to
d544ffb
Compare
scastlara
changed the title
scastlara
force-pushed
the
config-rewrite
branch
from
d544ffb to
9e0e212
Compare
scastlara
force-pushed
the
config-rewrite
branch
from
9e0e212 to
d9170af
Compare
| raise TOMLError(f"Error writing toml to {self._file_path}") from None | ||
|
|
||
|
|
||
| def _get_logging_level( |
There was a problem hiding this comment.
should this be inside the class?
There was a problem hiding this comment.
Since it does not use self at all, I just put it here.
| self.selector_method: Optional[str] = self._twyn_data.get("selector_method") | ||
| self.logging_level: Optional[str] = self._twyn_data.get("logging_level") | ||
| self.allowlist: set[str] = set(self._twyn_data.get("allowlist", [])) | ||
| def resolve_config( |
There was a problem hiding this comment.
I've been thinking for a while, could it be that this class' responsibilities are too broad?
For instance, right now this class both creates mechanisms to create config objects and interacting with the config file, would it make sense to split it?
Say, we have something that acts as a factory, just creating config objects, and then we have the config objects themselves, who are the ones that read and write to the file.
Maybe this is not the PR to make these changes, as some methods are mandatory both when creating the object and when interacting with the file (like _read_toml) but this could be adapted whenever we move this class' implementation to use the FileHandler.
There was a problem hiding this comment.
Overall agree that it would not hurt to separate the reading and the writing. Though I imagine they will need mechanisms to share stuff.
Having said that, this class has 3 public methods, and 150 lines or so. It's not too bad. I am not a fan of the config objects writing themselves though.
I do not really know how useful the filehandler will be for this, but I guess I'll see in the next PRs.
There are several problems (imo) with the
ConfigHandleras it is:initwhich makes it harder to use in tests, it's a strange side effect to read files on init.tomlkitas adict, buttomlkituses aTOMLDocumentclass (which acts somehow as a dict, but is not entirely adict).In this PR:
resolve_config(...)and you get back a frozen dataclass with defaults and everything you need to run Twyn. Pass in any cli arguments you have, andresolve_configwill know what to do. Defaults are handled here.remove_package_from_allowlistandadd_package_to_allowlistuse a new dataclass (ReadTwynConfiguration) that represents the configuration as parsed from the toml. Then they usetomlkit(not dictionaries) to write the toml document; handling whether thetwynsection is there or not.