-
Notifications
You must be signed in to change notification settings - Fork 995
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
docs(api): add description for coerce behavior for array type argument #1390
Conversation
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.
I agree with you about the wording.
I am afraid it would lead to a confusion between calling coerce for one key accepting an array of arguments, and calling coerce with an array of key (which is what I understood first reading your change).
I would suggest rephrasing it this way:
The coercion function should accept one argument, representing the parsed value from the command line (an array if multiple values are parsed for the key), and should return a new value or throw an error. The returned value will be used as the value for key (or one of its aliases) in argv.
👍 I like @mleguen's edit; mind making the update @yxliang01? |
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.
Thanks @yxliang01
The CI failing is weird, as it is coming from tests unrelated with any changes in this PR? |
The CI is failing on tests which are neither in the branch to merge, nor in the master branch. I find these failing tests in a commit from @laggingreflex, in its fork, for which I can see a CI build failure in Travis. @bcoe Is something wrong with Travis? |
@mleguen Yep. This only changes documentation - shouldn't affect CI result at all. I think for this PR, CI can be ignored? :) |
This fixes #1389 . The wording I feel is not so ideal. Maybe suggestions needed.