Skip to content
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

Fix Ini file settings ignored #669

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Fix Ini file settings ignored #669

wants to merge 8 commits into from

Conversation

ehooo
Copy link
Contributor

@ehooo ehooo commented Dec 14, 2020

Be cause the ArgumentParser args was set to a default value, the ini file is ignored always

Copy link
Member

@ericwb ericwb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the PR title to a consist meaningful message on what is being change.. Thanks!

@sigmavirus24
Copy link
Member

Arguably, this is the worst way to handle this. The defaults can no longer be rendered by --help and their values are moved somewhere completely apart from where they should be.

If we're going to move default values elsewhere, we should use argparse.SUPPRESS to more clearly signal that the option was not provided on the CLI altogether. That's the clearest and best way to accomplish what you want, even if it still hides and makes it impossible to introspect the defaults

@ehooo ehooo changed the title Fix issue #595 Ini file settings ignored Dec 21, 2020
@ehooo
Copy link
Contributor Author

ehooo commented Dec 22, 2020

@sigmavirus24 thanks for the information about argparse.SUPPRESS I never used it.
I just update the PR with that information.

@ehooo ehooo changed the title Ini file settings ignored Fix Ini file settings ignored Dec 22, 2020
@ehooo ehooo requested a review from ericwb December 22, 2020 15:58
@ehooo
Copy link
Contributor Author

ehooo commented Dec 22, 2020

@ericwb @sigmavirus24 All changes done.
Thanks for your feedback.

@sigmavirus24
Copy link
Member

So rather than relying on argparse.SUPPRESS to tell us whether something was specified, you're still setting it to something before using it which seems to completely defeat the purpose. That doesn't make a whole lot of sense to me

@ehooo
Copy link
Contributor Author

ehooo commented Dec 23, 2020

@sigmavirus24 i don't know any other way to detect if the user have enter the value via cli or not, if default value is set there is no any elegant way to know if there come from the sys.argvs or is the default value.
Or we set None as default or argparse.SUPPRESS in both ways we need set the default value when the ini file was processed, in other case the ini values will be discarded.

Please let me know how could i do for solve this issue? i'll happy to do it as you prefer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants