janluke / cloup

Library to build command line interfaces based on Click. It extends click with: option groups, constraints (e.g. mutually exclusive params), command aliases, help themes, "did you mean ...?" suggestions and more.
https://cloup.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
103 stars 10 forks source link

Suboptimal rendering of constraints in help output #182

Open mathrick opened 1 month ago

mathrick commented 1 month ago

Bug description

The way the constraints descriptions are printed under the option group headers doesn't work very well with longer descriptions that require wrapping. I have implemented a "required if missing" constraint as follows (RequireNamed is my custom constraint):

If(AnySet(*names), then=require_any.rephrased("optional"), else_=RequireNamed(*dependents))

and it results in the following output:

Asset metadata inputs:
  [optional if either --plugin or URL is set, otherwise parameters --title,
  --version, --godot_version, and --licence required]
  --title TEXT          Title / short description of the asset
  --version TEXT        Asset version
  --godot-version TEXT  Minimum Godot version asset is compatible with
  --licence TEXT        Asset's licence

Notice how the description wraps around in a way that makes it look like it's a part of the option listing. IMHO, there should be a blank line inserted between the description and the option list if the description wraps around.

To Reproduce

Steps to reproduce the behavior:

  1. Add the following constraint to any option group
    constraint=If("some_option", then=require_any.rephrased("optional"), else_=require_all.rephrased(
              "this is some very long description that will wrap around and make things look weird"
             ))
  2. Run with --help
janluke commented 1 month ago

Yeah, maybe an empty line could be added when the constraint is long.

Note that your constraint is probably wrong, as require_any doesn't mean "optional". In case your else_ branch actually require the full option group, the constraint could be rewritten as:

If(Not(AllSet(*names)), then=require_all)

which has a short description.

mathrick commented 1 month ago

Right, I realised it actually reduced to require_all as I was writing this bug report, but it doesn't take away from the substance of it, since I actually have other constraints where the list of names is a subset of the available options, so it still suffers from the readability issues. This just happened to be one that had especially confusing output.

janluke commented 1 month ago

Sure. But I'd recommend against using group constraints for a constraint that does NOT apply to the entire group. Use @constraint or the command help instead to instruct the user (or both). The correct use of an option group constraint is for constraint that apply to the full group and as such have a short description.