Move from "optparse" to "argparse".

On 2022-01-12, Adam Turner wrote:

Sorry for the radio silence!

No need to apologise. Docutils development is slow and there is real life, too.

I've made some progress in this branch

Looks promising, thank you.

I would not add as many leading underscores, though,
to keep consistency with the existing code base.
Exceptions are new additions that are bound to go/change in near future
or minor auxiliary objects that are only locally used.

Before the move to dict based arguments_spec, give us a pause to
plan/discuss/document what the specs shall look like.

The work does lead me to ask about how pace of deprecations / removal

That is a tricky question. I like clean code but I got burned several times
after removing obscure parts only to find out that this broke applications
with indirect dependency on Docutils and maintainers not knowing about the
project lest reading our RELEASE-NOTES :(

Can I remove the "old" config parsing logic? It was deprecated in 2003
in R1643 ( https://sourceforge.net/p/docutils/code/1643/ ).

Not yet, I am afraid. I changed the "may be removed" to "will be removed"
two months ago in [r8880]. Next step would be to add a removal version...

Equally, when deprecating features that would be obsoleted by my
changes, would you suggest announcing removal in v0.20.0, v1.0.0, etc?

I don't know whether there will be v0.20 at all. The idea is to reconcile the
reality (Docutils is used as if it were mature) and the version number (<1)
once the API sufficiently defined a deprecation policy is agreed.
Alternatively, 1.0.0 could be pushed off a while and be a clean start
(including removals currently scheduled for 1.2). This would require
re-wording the DeprecationWarnings to say 0.21 or later.

(I have seen the proposed deprecation policy -- my personal feedback
would be that it should go along with a versioning policy.

This discussion deserves a separate thread/ticket.

Thanks, Günter

Thank you for the reworked patch set!

It will likely be easiest to review commit-by-commit.

Lets start with 1/4

I do not see the benefit in the new helper functions outweighting the
refactoring effort:

Usage (after all 4 commits):

========================================= === ===== =========
new function lib tests dev-tools
========================================= === ===== =========
frontend.read_config_files 2
frontend.config_files_settings 1 1
frontend.get_default_settings 1 7 1
frontend.get_settings_with_defaults 1
frontend.parse_args 1
Publisher.config_section_to_settings_spec 3
========================================= === ===== =========

Making OptionParser.get_standard_config_files() a class method could be
combined with the removal of the mod_python hack.
The commit message or should note the reason
(mod_python beeing no longer developed and too old)
with a link to, e.g., https://en.wikipedia.org/wiki/Mod_python.

I see that after all 4 commits, frontend.OptionParser is still present
(is it also fully backwards compatible?)

Would it be feasible to have a minimal change where only the Values,
Option, and OptionParser classes were based on "argparse" instead of
"optparse"

Then we can start moving to frontend.ArgumentParser and using "argparse"
features step by step.

Sorry for the long delay. Here my update on patch 1/4.

On 2022-01-20, Adam Turner wrote:

The functions are of most use to downstream developers, I'd argue. It
provides a higher level abstraction which means you don't need to know
about the internals of how OptionParser &c work.

You don't need to know these details, when using the intended entry points:

• the framework provided in docutils.core for front-end tools and
  applications deploying docutils

• the readers.Reader, parsers.Parser, writers.Writer, and
  transforms.Transforms base classes for drop-in components.

For concrete use-cases where this interface is insufficient, we can work on
enhancement and improvement, but I don't want to start a parallel,
undocumented framework of "access functions".

... I was trying to move to a point where the OptionParser and
ConfigParser classes are only used in docutils.frontend --
providing a cleaner seperation of concerns and making any future
refactorings easier.

The frontend.ConfigParser is only used in frontend.OptionParser already.
Use of frontend.OptionParser will drop significantly with just one helper
function: frontend.get_default_settings(). Remaining cases can be handled
later (before deprecating the old OptionParser).

...

I think that is probably as minimal as it gets -- it was difficult
enough to rewrite everything keeping 100% backwards compatability --
and I can't make any assumptions about which names are "safe" to remove

We can check for usage in Sphinx and myst, this gives at least an indication.

I'll attach my proposal for a pre-conversion cleanup.

On 2022-01-25, Adam Turner wrote:

I am just working on a way to disentangle frontend.OptionParser and frontend.ConfigParser but this is a topic for bug:#441

Sounds good -- happy to review a proposed design.

My major sticking point is the cross-referencing: OptionParser uses a
ConfigParser instance and passes "self" to config_parser.read() because
ConfigParser requires an instance of OptionParser!

The reason is that some settings have side-effects (overwriting other
settings) that need to be resolved for each config file before reading the
next to keep the settings priority as expected. (cf. the attached
documentation)

My idea is to scale back frontend.ConfigParser to provide backwards
compatibility and the custom optionxform() while moving the
setting_validation() to frontend.OptionParser.

The OptionParser would then loop over the config_files,
read them one-by-one with config_parser(read(file)),
select settings from the "active" config file sections, and
"validate" them (including the overwriting of affected settings).

If you agree that this is a step in the right direction, I would try to
prepare a patch.

I'd propose making the --config flag simply store the path to the
file, and then add it to the list of config files to be read.

Mind, that the list of config files to read is processed before
parsing the command line.
An option parser based on "argparse" can use the parse_known_args() method
to pre-parse the command line, append additional config files and proceed.
...

Ideally, we would construct a single ArgumentParser object and add
arguments "normally" through parser.add_argument.
The problem here is that subclasses can filter settings_spec (through filter_settings_spec).
...
if settings_spec tuples were treated as immutable, then things would be much simpler.

Actually, both SettingsSpec instances and SettingsSpec.settings_spec
tuples do not "mutate" in Docutils.
filter_settings_spec() is only used in creating a settings spec tuple
from the settings specification of a base or "sister" class in the class definitions of the "xetex", "html4" and "html5" writers.
Other components ('pep_html", "s5_html") overwrite just some default values with SettingsSpec.setting_default_overrides and keep the parent's setting_spec.)

The Sphix "html" writer loops over the base class' settings_spec tuple
in order to set a default value in writers/html.py:59
https://github.com/sphinx-doc/sphinx/blob/4.x/sphinx/writers/html.py
This should be changed (before retiring the old format)
to use settings_default_overrides instead.

We could also move to a model (a la stdlib dataclasses where we
simply specify the setting name, type and default within the Component
itself.

This sounds like what I have in mind with a new data format to replace the
tuple SettingsSpec.settings_spec (mind, that Component inherits
SettingsSpec).

The front-end tools could then use that information (or a
mapping of settings to command line options or similar) to create the
CLI arguments.

I would keep the processing of settings specifications in the "frontend"
module with front-ends deploying them via the core.Publisher class.

I think this would be cleanest, but this cannot be done overnight.

But, maybe we should discuss more about a clean end state before
(or in parallel) with a step-by step review of the optparse-argparse switch.

Re-rebased
A

Attached a review and patch proposal.
More "high-level" planning and proof-of-concept files under sandbox/enhancement-proposals/settings-API