-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add table style for option formatting #25
Conversation
I'm afraid I cannot figure out how to fix mypy failure... |
First of all thank you for your on this PR! I as a user of this project would also highly appreciate table style options. After seeing your PR I looked into the mypy issue in effort to be helpful, but sadly as of now I could also not find a solution. In the process howerever I took a closer look at clicks usage parsing code. As a result I have a few notes in regards to implementation, but feel free to ignore this. It seems click uses For example for your test cases it would return:
For float range 7.* sadly does not have support for displaying the range yet, but the current alpha version of 8.0 supports this already:
Note how in contrast to choice and date the range is part of the help text and not the command + type part of the returned tuple. Also there is no indication of if Also something to note about the upcoming 8.0 release is that it will introduce Now my suggestion for the table feature is two fold:
**edit: note about custom support for FloatRanges Context Dict: Click to expand!{
"command": {
"name": "hello-table",
"params": [
{
"name": "debug",
"param_type_name": "option",
"opts": [
"-d",
"--debug"
],
"secondary_opts": [],
"type": {
"param_type": "String",
"name": "text"
},
"required": false,
"nargs": 1,
"multiple": false,
"default": None,
"envvar": None,
"help": "Include debug output",
"prompt": None,
"is_flag": false,
"flag_value": true,
"count": false,
"hidden": false
},
{
"name": "choice",
"param_type_name": "option",
"opts": [
"--choice"
],
"secondary_opts": [],
"type": {
"param_type": "Choice",
"name": "choice",
"choices": [
"foo",
"bar"
],
"case_sensitive": true
},
"required": false,
"nargs": 1,
"multiple": false,
"default": "foo",
"envvar": None,
"help": None,
"prompt": None,
"is_flag": false,
"flag_value": false,
"count": false,
"hidden": false
},
{
"name": "date",
"param_type_name": "option",
"opts": [
"--date"
],
"secondary_opts": [],
"type": {
"param_type": "DateTime",
"name": "datetime",
"formats": [
"%Y-%m-%d"
]
},
"required": false,
"nargs": 1,
"multiple": false,
"default": None,
"envvar": None,
"help": None,
"prompt": None,
"is_flag": false,
"flag_value": true,
"count": false,
"hidden": false
},
{
"name": "range_a",
"param_type_name": "option",
"opts": [
"--range-a"
],
"secondary_opts": [],
"type": {
"param_type": "FloatRange",
"name": "float range",
"min": 0,
"max": 1,
"min_open": false,
"max_open": false,
"clamp": false
},
"required": false,
"nargs": 1,
"multiple": false,
"default": 0,
"envvar": None,
"help": "The given help text",
"prompt": None,
"is_flag": false,
"flag_value": true,
"count": false,
"hidden": false
},
{
"name": "range_b",
"param_type_name": "option",
"opts": [
"--range-b"
],
"secondary_opts": [],
"type": {
"param_type": "FloatRange",
"name": "float range",
"min": 0,
"max": None,
"min_open": false,
"max_open": false,
"clamp": false
},
"required": false,
"nargs": 1,
"multiple": false,
"default": None,
"envvar": None,
"help": None,
"prompt": None,
"is_flag": false,
"flag_value": true,
"count": false,
"hidden": false
},
{
"name": "range_c",
"param_type_name": "option",
"opts": [
"--range-c"
],
"secondary_opts": [],
"type": {
"param_type": "FloatRange",
"name": "float range",
"min": None,
"max": 1,
"min_open": false,
"max_open": false,
"clamp": false
},
"required": false,
"nargs": 1,
"multiple": false,
"default": 0,
"envvar": None,
"help": None,
"prompt": None,
"is_flag": false,
"flag_value": true,
"count": false,
"hidden": false
},
{
"name": "help",
"param_type_name": "option",
"opts": [
"--help"
],
"secondary_opts": [],
"type": {
"param_type": "Bool",
"name": "boolean"
},
"required": false,
"nargs": 1,
"multiple": false,
"default": false,
"envvar": None,
"help": "Show this message and exit.",
"prompt": None,
"is_flag": true,
"flag_value": true,
"count": false,
"hidden": false
}
],
"help": "Hello, world!",
"epilog": None,
"short_help": None,
"hidden": false,
"deprecated": false
},
"info_name": None,
"allow_extra_args": false,
"allow_interspersed_args": true,
"ignore_unknown_options": false,
"auto_envvar_prefix": None
} |
@yudai-nkt OK I have now found the reason and a fix for the mypy errors The problem seems to stem from the fact that the typeshed for click types (third party type hints) is missing the type hints for the attributes. e.g., FloatRange is currently defined as class FloatRange(FloatParamType):
def __init__(self, min: Optional[float] = ..., max: Optional[float] = ..., clamp: bool = ...) -> None: ... but should be class FloatRange(FloatParamType):
min: Optional[float]
max: Optional[float]
clamp: bool
def __init__(self, min: Optional[float] = ..., max: Optional[float] = ..., clamp: bool = ...) -> None: ... Now this means to ultimately fix the problem one would have to make a PR for typeshed and add the missing attribute type hints in |
I don't have much time now (it's 2:00 am here and I was about go to bed haha), but I'd like to answer to some of your comment which I can react to right now.
I actually once used
Range support in v8 seems good, but I like being verbal (i.e., between, above, and below) rather than using mathematical expression given that HTML documentation has more room than terminal emulator. It's just a matter of preference of course. Thank you for your suggestions and information anyway, @max-frank! I'll read it thoroughly later and consider which direction I should go (pallets/click#461 especially looks quite promising). |
I was suspecting this was the case. I should have also looked at your old PR sorry.
Indeed its just a matter of preference one thing to note though starting with v8 FloatRange and IntRange will support configuring if the range is inclusive or exclusive for both ends. Just something to keep in mind so that the code will be easily adaptable for future 8.* support. お疲れ様 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! This is looking good. :)
I left several comments on the PR.
For comments marked as "(Nit)", I'm happy to ignore those for now and get back to it later with a follow-up cleanup PR. Other comments are a bit more blocking to merging this though.
@max-frank Great one on submitting a PR to @yudai-nkt For now we can just |
@florimondmanca I updated the branch according to your comments. @max-frank After some thought, I'd like to keep the initial direction but will try the v8 features you introduced me after the stable version is released. Then I'll submit a PR if it turns out to be worth it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great! Some last-round questions / suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems ready to me. I have some cleanup ideas but I'll treat that as a follow-up.
Thanks so much for your work on this @yudai-nkt and @max-frank for reviewing. :-)
Rewrite of #12 in a backward-compatible manner.
One difference from what @florimondmanca and I discussed is that I named the option
:option-style:
. This is more descriptive thanstyle
and easy to extend when we want to define styles for other sections such as usage.Fix #11.