FastEnum: implement __str__ and __repr__ to match StrEnum for docs reproducibility

[tti0]

I have been looking to package BuildStream's documentation alongside the application and library in Debian. Debian policy now requires that binary packages, including documentation, are reproducible, so this MR is motivated by fixing the (only) reproducibility failure Debian CI discovered in BuildStream's documentation, in the Using / Commands page.

This is my first upstream patch to BuildStream, so I am of course happy to adjust as needed. Thank you for your consideration!

---

sphinx-click uses __repr__ when rendering the default option of a click.choice in the HTML documentation [ref].

Implementing __repr__ on _PipelineSelection gives more helpful (and reproducible) documentation than the default representation, <buildstream.types._PipelineSelection object at 0x7f775b6b4ec0>. As an example in the documentation of bst show:

Mainline:

This branch:

[juergbi]

Thanks for the patch. This looks like a good documentation improvement even ignoring reproducibility.

One might argue that sphinx-click using repr() (which is targeting developers) for command line argument defaults is wrong. This was introduced in click-contrib/sphinx-click#137 and there is a somewhat related issue open about this: click-contrib/sphinx-click#154.

The default repr() for FastEnum is pretty bad, though, so implementing __repr__ makes sense even if sphinx-click didn't use it. However, if it wasn't for sphinx-click, I might want __repr__ to work more like StrEnum as seeing the enum type and not just the string value in debug output may be useful. That would be something like <buildstream.types._PipelineSelection.ALL: 'all'>, which would be informative for debugging (and also reproducible) but still not optimal for the documentation.

One possibility would be to implement FastEnum.__repr__ to match the StrEnum output but switch to strings for click.option defaults for more readable documentation:

@click.option(
    "--deps",
    "-d",
    default=str(_PipelineSelection.ALL),
    type=FastEnumType(
        _PipelineSelection,
[...]

I haven't properly tested this but I think click will call FastEnumType.convert() on the default, so the default is allowed to be a string. If this doesn't work, we could alternatively add show_default=str(_PipelineSelection.ALL).

[tti0]

Thank you for taking the time to review this @juergbi.

I have pushed a new version adjusting the __repr__ and __str__ representations for FastEnum itself to match CPython's StrEnum: the default documentation renders satisfactorily (as 'foo' rather than foo though) and tests pass locally. I'd appreciate a more thorough check that this change doesn't have any other breakages as side effects.

Thanks again.

[juergbi]

You missed one case on line 1676 of cli.py. Looks good to me overall, thanks for the update.

[tti0]

Apologies, I have applied both these suggestions now and pushed a new version. Thanks for taking the time to review this further.