juergbi commented on PR #2139: URL: https://github.com/apache/buildstream/pull/2139#issuecomment-4933772339
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 https://github.com/click-contrib/sphinx-click/pull/137 and there is a somewhat related issue open about this: https://github.com/click-contrib/sphinx-click/issues/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)`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
