config: add support for ini option aliases - #13831
Conversation
Sorry, something went wrong.
nicoddemus
left a comment
There was a problem hiding this comment.
Nice work!
Sorry, something went wrong.
| assert config.getini("old_name") == "value1" | ||
| assert config.getini("legacy_name") == "value1" | ||
|
|
||
| def test_addini_aliases_with_override(self, pytester: Pytester) -> None: |
There was a problem hiding this comment.
Perhaps worth adding a test where the ini file uses the new name, and the cmdline override uses the old name.
Sorry, something went wrong.
There was a problem hiding this comment.
Great suggestion -- this case is actually broken. It worked in my version before #13830 but broke after it.
I pushed the broken test now, will work on the fix later.
One option is to revert #13830, but the version before was pretty unruly, I still think #13830 is a good idea.
My thinking is to track the origin of ini values in inicfg (change it from dict[str, str] to dict[str, IniValue] where IniValue contains the value and origin -- configuration file or cli override). Then we give priority to overrides even if they're alias and the canonical is defined. Maybe the origin can be used for better error messages as well.
Sorry, something went wrong.
There was a problem hiding this comment.
Sounds good. 👍
Sorry, something went wrong.
There was a problem hiding this comment.
Implemented this, pushed as a separate commit (will squash before merging).
Sorry, something went wrong.
nicoddemus
left a comment
There was a problem hiding this comment.
LGTM!
Sorry, something went wrong.
|
Btw your IniValue solution gives me the impression that augmenting it by adding the aliases to that dataclass might improve the code further (we would have a single class defining the values and the aliases, instead of that information being split in two dicts) -- but I have not thought about it in detail, plus it is a larger refactoring which I'm not sure you are willing to tackle now. |
Sorry, something went wrong.
|
We want an alias -> canonical mapping, because it's used by the cached getini which should be quick. So I think it wouldn't end up better. |
Sorry, something went wrong.
Fix #13829.
Currently based on #13830 -- please skip that commit.