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

Add an option dont_prefill_defaults to ArgParseSettings #47

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fjarri
Copy link

@fjarri fjarri commented Aug 5, 2017

This option would be helpful in the following usage scenario.

Suppose that, in addition to command-line arguments, the options can be supplied via function arguments, or a config file. One would want the command-line options to have the higher priority than that, so if an option is supplied from the command line, it overrides the config file value, or the value hardcoded in the program. This seems like a logical requirement, since the command-line options are the most "volatile" ones (they do not require re-compilation or changing some external file).

This option helps to implement this kind of logic. Currently it is impossible to tell from the return value of parse_args() whether the value for an option was actually supplied by the user, or taken from the default value (and, therefore, should be looked for in a config file).

@fjarri
Copy link
Author

fjarri commented Aug 5, 2017

Honestly, I'm a bit stumped by the CI fails... It's just a segfault in the middle of the test for the julia-nightly versions. I am not sure what can be causing this, any ideas?

@fjarri
Copy link
Author

fjarri commented Aug 5, 2017

The segfault happens without my patch as well. Does not seem to be an ArgParse bug, filed an issue on Julia itself

@carlobaldassi
Copy link
Owner

Sorry for being so late in looking into this, it slipped under my radar for quite a while.

I don't fully understand the use case though. It seems to me that what you want are options whose default is "look in the config file". Couldn't you just use some sentinel value for that, e.g. make the default value for an option be nothing, in which case you look in the file?
The only case where this is not possible, it seems to me, is for the case of flag options with the :store_true or :store_false action, but in that case you could just use the :count_invocations action instead, for example.

Alternatively, and perhaps better in some cases, you could set the defaults from the config file itself, using the function form of add_arg_table (that is unless you want to provide the config file as an argument too...).

Am I missing something?

@fjarri
Copy link
Author

fjarri commented Mar 28, 2018

Yes, you understand correctly. It is possible to implement the required functionality with the existing API. In fact, I am using the second method you suggested (passing the config values to the function that builds the argument table to be used as defaults). I do not particularly like this approach because

  • the argument table builder gets access to the information it doesn't actually need
  • I have to maintain a dictionary of the "primary" defaults to be used as a fallback if neither the config nor the commandline arguments specify a parameter
  • Boolean parameters require special processing (with :count_invocations or otherwise)

It just seems overcomplicated to me, so I thought that an option that allows one to process the config and the commandline arguments separately and join them later would be useful.

@carlobaldassi
Copy link
Owner

I see. Maybe a more general feature would be to have an internal list associated to a special key ("%PARSED_ARGS%", for example) that lists what options were passed? I think the most general would be a sort of structured log of the parsing.

@fjarri
Copy link
Author

fjarri commented Apr 10, 2018

I guess that will work as well. I am not sure what you mean by the structured log though.

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.

2 participants