CLI options must be passed all the time

[lemon24]

...which requires either duplicating them everywhere (so when things change, they need to change in more than one place), or having some alias/wrapper (harder to do for subcommands), or just living with it.

[lemon24]

The obvious solution is to use a configuration file.

Some thoughts:

At least initially, configuration loading should not be part of the core; most of the things that are configured at the moment are outside of it. If needed, we can add a reader_from_config function later.

The same config file should work with the WSGI app (reader._app.wsgi:app), where applicable.

There should be a 1:1 mapping between the CLI options and the config file; both pip and uWSGI do this.

• It is acceptable to have all options available in the config file.
• We don't need a config file hierarchy, like pip does (local/user/site).
• We may get away with only having a single environment variable, that for the config file. If it's easy to do, we may have them for all the options. (At the moment, we have READER_DB, READER_PLUGIN and READER_APP_PLUGIN, but we have them because they were required for the WSGI app.)

We should keep the amount of dependencies to a minimum.

• Whatever format the stdlib configparser supports should be OK.
• There are some libraries that build upon Click that we may use:
  • https://github.com/click-contrib/click-configfile
  • https://github.com/phha/click_config_file
  • see https://serge-m.github.io/click-config-parsers.html for a comparison, and a Stack Overflow snippet that does similar things

Things that we need to configure at the moment:

• database URL / connection string
• {storage,search}_cls and {storage,search}_option (basically, whatever arguments make_reader takes – Support multiple storage implementations #168)
  • --debug-storage (this should likely be a storage option)
  • enable / disable WAL (not exposed at the moment) (should this be handled through reader? why not let the user do it themselves? update: it reads 32k on every database open: Run "PRAGMA optimize" on close() #143 (comment))
  • note that the CLI and web app may need different values (e.g. optimize_on_close in Run "PRAGMA optimize" on close() #143 (comment))
• update workers
• serve hostname and port (meaningless for the WSGI app)
• plugins
  • reader plugins
  • web app plugins
    • also reader plugins when reader.update_feed(s) is used from within the app (e.g. /preview, or the preview_feed_list plugin)
  • plugin options (not needed now, but may be for Plug-in API #80)
• --verbose (per-command and global; the CLI doesn't have global at the moment)
• search state (enabled/disabled); currently, we have subcommands for this (search enable / disable), not a flag
  • maybe make_reader() should have an enable_search option
• feature flags, maybe (example)

[lemon24]

Here's a prototype of a config file format and how it would be used: link.

Update: prototype updates:

• canonicalized CLI defaults/options layering a bit, link
• added a wrapper to make using the config more friendly, link
• another, possibly better way of tracking defaults, link; can't have bool defaults, though
  • workaround for the bool defaults thing: link
• TOML version of the config, link
• alternate configuration structure, link, see CLI options must be passed all the time #177 (comment) below for details

Some notes about it:

• It would add about 100 lines of code to integrate.
• It feels like it does too much.
• It doesn't provide fancy validation (the error messages will be pretty crude).
• It assumes all arguments are already converted (with the exception of plugins, which it knows how to import).
• It has support for the following:
  • reader/cli/app config sections
  • merging config sections
  • providing defaults to the CLI

Some general notes:

• The CLI doesn't overlap entirely the settings reader needs, nor should it. Not everything needs to be configurable from the CLI.
• I changed my mind about the config file format; having types / not having to convert the types of things explicitly is definitely a plus. YAML or TOML it is.
  • Update: TOML doesn't have the equivalent of None, and likely won't get one ever (nil or null values toml-lang/toml#30, In favor of NULL toml-lang/toml#146).
    • A possible workaround is to use an empty dict
      • for plugin config that's great, since you can write that as an empty section
      • for make_reader(feed_root=...) (Handle non-http feeds gracefully in the web app #155, which is None or str), it would need special handling (and it would look ugly and confusing), and we need None, especially given that in 1.0 it is not the default
        • we could use False instead of None in make_reader_from_config only
          • this won't work for storage_wal_enabled, which is Optional[bool]
    • Maybe cut another issue for this, listing as use cases feed_root, and merging config sections (with None used to unset keys).
    • If we do end up using TOML, we should use the tomlkit package (preserves comments, sorts sections in a sane way), not toml (has weird spaces and commas in places, sections are sorted weird).
• The prototype above assumes the plugin loading protocol has a way of knowing if something's a reader, cli or app plugin, and just ignores them otherwise. Example:
  • for import path package.module, the reader plugin is package.module:init_reader
  • for import path module:Object, the app plugin is module:Object.init_app

[lemon24]

Some thoughts regarding the TOML thing above:

We use Optional types in various places throughout the API in a way where None doesn't necessarily mean "absence of a value" or "use the default", but rather as a special value with a different meaning:

• make_reader(feed_root: Optional[str]) – str means "use this path as root", None means "no local feed parsing entirely"
• Storage(wal_enabled: Optional[bool]) – bool means enable/disable, None means don't do anything
• Reader.get_entries(read: Optional[bool], important: Optional[bool], has_enclosures: Optional[bool]) – bool means only (un)read etc., None means all (== don't filter)

The docs say None is frequently used to represent the absence of a value, as when default arguments are not passed to a function. Our usage abuses this meaning somewhat, and this may pose some issues in the future.

So, regarding the current usage:

Pros:

• Easy to use API, no need to import any enum or remember any enum-like string.
• In some ways, it maps well to the meaning of "don't do anything".
• It's not clear what the replacement is:
  • Optional[bool] can be replaced by an Enum.
  • Optional[str] could be replaced by an enum with data attached, like Rust has.
    • Update: Here you go: packages with mypy support: algebraic-data-types, tagged-dataclasses; mypy recipe from the docs, mypy issue.
  • Make an alternative None, and use Union[AlternativeNone, type].
    • Update: Here you go: gist.
    • Another update: We already have one, reader.types.MISSING.
• Update: Hypothesis does it with hypothesis.strategies.ip_addresses.

Cons:

• It prevents using None as meaning "do the default thing"; of course, you can just not use that kwarg to achieve this, but it's not possible/easy to propagate across calls (you don't always control how the function is called).
  • Another example of this is in a config file, where section B overrides section A; you can't use None in B to mean "disregard whatever A said, use the default".
• It prevents interoperability with things that don't have a concept of null (e.g. TOML) or that don't (make it easy to) distinguish between true absence or a value being null (e.g. Lua tables or some interpretations of URL query strings).

Not a concern:

• Optional[bool] being easy to misuse (e.g. using if thing instead of if thing is False). You can do that with an Enum as well (with a slightly different meaning).
• The CLI, for optional strings. Example: To fully model feed_root currently (with the empty string default), one would need two flags: --feed-root /path (string option), and --no-feed-root (bool option); the options need to be mutually exclusive. To model it if it used the Union[AlternativeNone, str] proposition would need the same amount of effort.
  In conclusion, it may be nice to stop doing this if we can find a decent replacement. Obviously, it would have to be done in 2.0 to actually get rid of the cons.

[lemon24]

To implement the prototype:

• add config stuff to _config.py
• integrate into _cli.py without fancy config support
• add fancy config support to _cli.py
• integrate into _app/wsgi.py
• add dependencies to setup.py
• [ ] (maybe) remove unneeded envvar support (from both CLI and app)
  • they're useful for debugging, keep'em
• (maybe) add a special Config object to remove some of the boilerplate
• add some basic tests
• document

[lemon24]

Added an alternate configuration structure and parsing code to the prototype gist: link.

Changes/improvements:

• Not make_reader_from_config-arguments-centric; the app/cli config section has a section with reader overrides, not is a section with reader overrides.
  • We'll postpone actually implementing overrides thing until we actually need them.
• Reader plugins and app plugins are in different, unrelated lists; this is consistent with the current usage, so we have to implement fewer things (e.g. no changes to plugin infra).

[lemon24]

The test added for the whole config thing in bfb7dfc shows the prototype doesn't actually work like we thought it does.

In general, because the way int parameters work (the value always gets passes to int), we can't wrap the defaults in anything.

We'll have to find a different way of seeing if an argument is a default or a user value.

Maybe override Context.lookup_default()? (but this would only take care of the default_map values...)

• to use a different context class:
  • in Click 8.0, BaseCommand.context_class
  • in 7.0, it's hardcoded

Update: I think this describes the same problem: pallets/click#549 (comment)

[lemon24]

Yeah, nevermind, removed the whole thing: ac2c173

[lemon24]

READER_CONFIG in use: lemon24/owncloud@b6a6ba2