Refactor config file parsing

[lahwaacz]

This is a basic refactoring to address #21, I'm working on testing it with custom parser now... All tests are passing, so hopefully I did not mess up anything.

The documentation and examples in the README will be updated based on input from others.

[lahwaacz]

There is a couple of undocumented features/regressions I wanted to discuss:

1. The documentation says that one of the design choices was that "all options must be settable via command line". But specifying default_config_files in the ArgumentParser constructor makes it possible to have a config file path that is not settable via command line.

   I'd suggest to hold the design choice and solve this with dropping the constructor parameters and rely only on the add_argument() method, see next points for details.

2. Currently all default config files are parsed and values from user-specified files are placed on top. I find this behaviour rather unpractical for a library, because this way values from the default config files cannot be unset by the user. An alternative is to read only one default config file (the last one), and when the user specified an alternative config file path, don't read default configs at all. This is loosely coupled with 1.

3. It is theoretically possible to set default config path per-argument in the add_argument() method, for example:

   parser.add_argument("-c", "--config", default=["foo.conf", "bar.conf"],  is_config_file_arg=True, help="config file path")

   IMO this is more flexible than specifying everything in the constructor, but it is not supported by ConfigArgParse yet (at least the default config files specified this way are handled differently from those specified in the constructor).

I have a prototype addressing the above issues in my config_files_without_merging branch, but since it breaks stuff (including some tests), it should be discussed before merging into master.

[bw2]

Thanks for the great pull request!

Also, I need to go through and better understand the config_files_without_merging changes, but I like the idea of enabling add_argument( is_config_file_arg=True .. default="foo.conf"), but would prefer to also keep the current constructor-settable approach (for convenience + backward-compatibility). I don't see an issue with supporting both. (In retrospect this isn't a great analogy, but I originally based the behavior on how .bashrc is sourced at startup - first system then local).

I'm more hesitant about enabling multiple default files in the arg (eg. default=["foo.conf", "bar.conf"]) since it's type _StoreAction so users would expect the default to be a single value, and also I'm guessing the need for multiple files here is rare.

What do you think?

[lahwaacz]

Enabling multiple default files in the add_argument method makes it more consistent with the constructor-based approach. I don't see any other way to allow specifying multiple default values (of which only the one highest priority is actually used) while enforcing the command-line argument to accept only a single value. If the behaviour is properly documented, I don't see a problem with this approach.
The purpose of this behaviour is to allow writing e.g. default=["/etc/foo/foo.conf", "~/.config/foo/foo.conf"] and make e.g. --config bar.conf valid, but --config bar.conf --config baz.conf invalid. IMHO this is a very common behaviour situation. Also note that as per the second point above, only one of the multiple supplied configs should be read.

[bw2]

>>> Enabling multiple default files in the add_argument method makes it more consistent with the constructor-based approach.

• I like the idea of allowing default value(s) for config file args - not sure if this would even be visible, to the user, but perhaps the config file arg's "action" should to change to "append" instead of "store" so that a list value makes sense as default?

>>> re: "Also note that as per the second point above, only one of the multiple supplied configs should be read."

• Not being able to unset system config file args seems like the more standard / logical behavior to me (the same way that command line args can over-ride but can't unset config file args).
  If not being able to unset is the main issue here, what if we just come up with a config file syntax
  for "unsetting" an arg (eg. name=NULL or name=None)? It could also be used on the command line (except for flag args), and reduce the need for changing the config file loading behavior. I've seen tools that support this type of unsetting (eg. Picard), so it wouldn't be totally unprecedented.

>>> while enforcing the command-line argument to accept only a single value. If the behaviour is properly documented, I don't see a problem with this approach. The purpose of this behaviour is to allow writing e.g. default=["/etc/foo/foo.conf", "~/.config/foo/foo.conf"] and make e.g. --config bar.conf valid, but --config bar.conf --config baz.conf invalid.

• I'm not sure I understand the value in preventing people from doing --config bar.conf --config baz.conf?

[lahwaacz]

There are too many (sometimes even conflicting) conventions. For sure the configargparse library should not be too restrictive and let the application developers choose the most fitting convention for their use case.

Actually, the action="store" for config arguments is already enforced, but it does not check the default= parameter. I guess the argparse devs did not forbid combining action="store" and default=[...] for a reason, after all it is not the end users that use the library. I think that releasing the requirement on action="store" for config arguments to accept also action="append" would be more fitting for the library.

Rather than introducing null values, which may depend on the config file format, perhaps we should add another parameter for the ArgumentParser class to specify how many configs should be read at most? I can think of many programs for both sides of the argument (e.g. makepkg sources ~/.config/pacman/makepkg.conf on top of /etc/makepkg.conf, but in e.g. dhcpcd or dnsmasq the user configuration overrides the default) and it would be a shame if configargparse is too restrictive.

[bw2]

Oh, I forgot about that. I can switch it to accept Append also.

Looking at dnsmasq (http://www.thekelleys.org.uk/dnsmasq/docs/dnsmasq-man.html#lbAF) the behavior generally differs from configargparse:
"For options which may only be specified once, the configuration file overrides the command line. "
so I'm not sure I want to emulate it.. and I'm still not fully understanding the use-case where you need to specify multiple config files, but then discard one of them. Are there other tools besides these network ones that use this behavior?

>>> add another parameter for the ArgumentParser class to specify how many configs should be read at most
would you ever need more than 1 but fewer than all files loaded? If not, could this be a boolean (eg. load_one_file)? and then, should it be the last file found or the 1st file found?

[lahwaacz]

Looking at dnsmasq (http://www.thekelleys.org.uk/dnsmasq/docs/dnsmasq-man.html#lbAF) the behavior generally different from configargparse: "For options which may only be specified once, the configuration file overrides the command line. " so I'm not sure I want to emulate it.. and I'm still not fully understanding the use-case where you need to specify multiple config files, but then discard one of them. Are there other tools besides these network ones that use this behavior?

Modifying the behaviour to match dnsmasq _exactly_ was not the point. Of course there are more than just dnsmasq: i3, i3status, mpv, mpd, pulseaudio -- I could most likely go on. The programs doing it this way are not restricted to a single category and neither should ConfigArgParse.

would you ever need more than 1 but fewer than all files loaded? If not, could this be a boolean (eg. load_one_file)? and then, should it be the last file found or the 1st file found?

You might e.g. want to skip all system configs if any user config has been found, but you're right that specifying this by a fixed number is not very useful.

[bw2]

I don't mean to beat this to death, but I went looking for how this behavior is documented in another tool, but couldn't find a good description. For example https://wiki.archlinux.org/index.php/Music_Player_Daemon
has one section on /etc/mpd.conf and one on ~/.config/mpd/mpd.conf, but I didn't find docs on what happens when both files are present. Same for the other tools.

Also, to play devil's advocate, in the case where you want only one file to load, why not code this up just prior to defining configargparse args. eg.

default_config_files=[]
if os.path.isfile("~/.bla.conf"):
   default_config_files.append("~/.bla.conf")
elif os.path.isfile("/etc/bla.conf"):
   default_config_files.append("/etc/bla.conf")

then pass default_config_files either to the constructor or to add_args(.., default=)

[bw2]

#25 might also be relevant to this. and it might be good to move further discussions to a new Issue.

[lahwaacz]

Regardless of what the documentation for the mentioned programs says, I've actually tried it or looked into the source to confirm that only one config file is loaded.

If one wants to always load only one file, including the one specified to e.g. --config option, there is no other way than doing this properly in ConfigArgParse.

[bw2]

@lahwaacz After thinking about this more, I would prefer (assuming there's broader interest) to add null values rather than add a new / different config-file-loading behavior that would coexist with the current one. Since (as you pointed out) default="bla.txt" already works for config file args, that's as far as I currently want to go in this direction.