Re: [Docutils-develop] [docutils:bugs] Re: #441 Move from "optparse" to "argparse".

[Guenter M. <mi...@us...>]

On 2022-02-05, Adam Turner wrote:

> High level review:

> - The header states the GPL licence rather than public domain -- would
>   you be happy to say public domain instead?

This is from my standard template for new Python files. For Docutils, I use
the 2-clause BSD for new files and the orignal license for patches to
existing files.

> - Strong negative on validation within ConfigParser. I think we should
>   make validation distinct from parsing, but that should be a different
>   change

A strong point for validation in the parse stage is, that you can
have per-configfile error messages/warnings. It is suboptimal when an
invalid value (a negative interger, say) could be from any of the
configuration files + command line but the warning/error does not tell.

Per-file warnings are also possible, if the arg_parser calls the
ConfigParser instance in a loop::

    for cf in configuration_files:
        cf_parser.read(file)
	validate(cf_parser['ACTIVE'])
	
The alternative concept would be a "stand-alone" ConfigParser that would:

* parse SettingsSpecs for active sections, validation functions,
  and side-effects
* read config files
* merge active settings from active sections
* convert and validate the values

This way, applications that don't parse the command line would not need to
set up an ArgumentParser instance at all.

For this, the new SettingsSpecs.argument_spec format should be:

* easy to code/read and understand in the component files
* easy to convert into `ArgumentParser.add_argument()` calls.
* easy to parse for active sections, validation functions,
  and side-effects in the ConfigParser.
  
A common utility would convert the current SettingsSpecs.settings_spec
format into the new one.


> - I think the "ACTIVE" section could be a method or an `@property` --
>   `ConfigParser.active_config` or suchlike, that would be accessed by
>   `OptionParser`.

I would prefer to stay with the common interface from "configparser":

1. set up <instance> (here, also pass info about components)
2. <instance>.read()  (this will also merge the settings)
3. Get the data as <instance>[section] (or <instance>.<dict instance attribute>).

> - Explicit side effect handling -- this is fine but we should document
>   that only "XXX" is allowed to have side effects for backwards
>   compatability purposes, and ideally we would deprecate the idea of
>   side effects.

I would keep the additional "overrides" keyword as well as the
"validate_encoding_and_error_handler()" action open for use in
settings-specifications. Maybe discourage its use, though.

> - let me know if you want a review of the specific implementation
>   (there's a few things I'd change but I don't want to derail the
>   discussion)

A peer-review of the implementation would be welcome. You may send a
patch/link in private mail or just mention me at github to keep the
discussion here more on the high-level aspects.

Thanks, Günter