-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
config: add support for ini option aliases #13831
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
testing/test_config.py
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps worth adding a test where the ini file uses the new name, and the cmdline override uses the old name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented this, pushed as a separate commit (will squash before merging).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Fix #13829.
Currently based on #13830 -- please skip that commit.