Prepare Loader and MemoryLoader for TOML support #3344
Conversation
| def __init__(self, **kwargs: Any) -> None: | ||
| super().__init__(Section(prefix="<memory>", name=str(id(self))), []) | ||
| self.raw: dict[str, Any] = {**kwargs} | ||
| def __init__( |
There was a problem hiding this comment.
This is a backwards incompatible change and cannot happen.
There was a problem hiding this comment.
OK, we could have cleared this in #3309. Anyway, he are reasons why I did it:
MemoryLoaderis not listed in API docs: https://tox.wiki/en/latest/plugins_api.html Hence, I assumed it may have incompatible changes.MemoryLoader.__init__isn't currently compatible with the documented API forLoader.__init__.
I can add some compatible layer and/or deprecation warnings, but I need to know how to handle it correctly. More incompatible changes will be needed for TOML support.
There was a problem hiding this comment.
MemoryLoaderis not listed in API docs: tox.wiki/en/latest/plugins_api.html Hence, I assumed it may have incompatible changes.
Oversight.
MemoryLoader.__init__isn't currently compatible with the documented API forLoader.__init__.
By design. It is meant to be instantiated with the appropriate values. Why would a child class be compatible with its parent constructor? The substitution principle works in the other way around, I believe.
There was a problem hiding this comment.
The compatibility makes the child class more flexible, especially when some of the omitted arguments become needed such as in this case.
Now, I'm not quite sure which way use to make it compatible. It seems to me it would be best to create new MemoryLoader class with modified constructor and deprecate current one. Is that OK?
There was a problem hiding this comment.
I disagree. Why does a memory loader need to know about its section? Or overrides. These are concepts that do not make sense in this context, so I am not sure why you are trying to force it into?
There was a problem hiding this comment.
If the MemoryLoader is part of public API it should we able to handle various sorts of configs, such as complete config of tox. In such case, it needs to know which section it should extract the key from and whether to apply any overrides.
Or do I not get the purpose of MemoryLoader correctly?
There was a problem hiding this comment.
I hope you reach an agreement here. I see the reasoning for having to access these bits, we use it in tox-ansible plugin. At the same time, I would try to avoid breaking changes (changing signature does not always count as breaking in my eyes).
There was a problem hiding this comment.
I feel like overrides should be their own loader 🤔 and not depend on the config source. 🤔
There was a problem hiding this comment.
Ok, I looked into this, overrides are intended to be not enabled for a memory loader. And I guess what I am not understanding is why you want to change that? The TOML loader should not be a memory loader.
There was a problem hiding this comment.
Here is an implementation demonstrating there is no need for this #3353, will continue over the next week to get it in a ready to ship state 👍
|
Replaced by #3353 |
tox -e fix)docs/changelogfolderupdated/extended the documentation