[WIP] [1140] use longest option argument again #1149
Conversation
|
WIP for:
|
click/types.py
Outdated
| @@ -489,7 +489,7 @@ class Path(ParamType): | |||
|
|
|||
| def __init__(self, exists=False, file_okay=True, dir_okay=True, | |||
| writable=False, readable=True, resolve_path=False, | |||
| allow_dash=False, path_type=None): | |||
| allow_dash=False, path_type=None, converter=None): | |||
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
altendky
force-pushed
the
1140-use_longest_option_argument_again
branch
from
fe85a3a
to
8c80220
Compare
altendky
force-pushed
the
1140-use_longest_option_argument_again
branch
from
ea3daba
to
090f0ad
Compare
altendky
changed the title
altendky
changed the title
|
Just wanted to echo your earlier #1140 (comment) here as well:
👍 |
There was a problem hiding this comment.
@altendky, is this still a work in progress?
Since this is the course of action which you and @davidism agreed on, and the changes appear to be complete to me, I'd like to merge.
I agree that your commit messages are a bit minimal -- personally, I'd prefer them either squashed or fleshed out with explanation -- but all of the important discussion is in GitHub anyway.
| @pytest.mark.parametrize('option_args,expected', [ | ||
| (['--aggressive', '--all', '-a'], 'aggressive'), | ||
| (['--first', '--second', '--third', '-a', '-b', '-c'], 'first'), | ||
| (['--apple', '--banana', '--cantaloupe', '-a', '-b', '-c'], 'apple'), | ||
| (['--first', '--second', '--third', '-a', '-b', '-c'], 'second'), |
There was a problem hiding this comment.
This makes me so dizzy to read. 😂
I understand that we're going back to documented behavior, but this is pretty much the perfect example of why I think this comment of yours is spot on. We should really just tell people to write down what they mean...
|
After #1140 (comment) and #1140 (comment) I figured we should figure out where we are going since we have no totally obvious thing to revert to. If we want to 'just revert' for now I would think we would want the previous functionality which would be 'last long' not 'longest' like this implements. |
|
Ah, I thought after this comment that we should revert the behavior that this reverts #794. Reading that PR again, it looks like we switched from "last long opt" to "first long opt" but never updated the docs? I didn't realize we never had the also-documented "longest option" behavior. Sorry for jumping in on this without properly grasping the situation. Now that I do, I actually think this PR is probably a bad call -- we should either keep the new behavior in 7.0 and document it, or revert to pre #794 behavior and note it as a bugfix in the 7.1 changelog. IMO, keeping the new behavior and just updating the docs is the cheapest/easiest solution. If someone showed up as a new user of 7.0, then any behavior change appears to be breaking. |
|
I only left this open to avoid thrash just in case... But yeah, this is likely just totally dead. |
#1140