Uh oh!
There was an error while loading. Please reload this page.
[WIP] [1140] use longest option argument again - #1149
Conversation
WIP for:
|
| 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.
Uh oh!
There was an error while loading. Please reload this page.
fe85a3a to
8c80220Compareea3daba to
090f0adCompare
amiryal
left a comment
There was a problem hiding this comment.
My only feedback, other than LGTM 👍, is regarding commit messages. Maybe you could provide more context, i.e. what the change actually does and why, and possibly squash some or all of them together.
amiryal
commented
Oct 23, 2018
Just wanted to echo your earlier #1140 (comment) here as well:
👍 |
sirosen
left a comment
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.
| (['--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...
altendky
commented
Nov 10, 2018
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. |
sirosen
commented
Nov 10, 2018
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. |
altendky
commented
Nov 10, 2018
I only left this open to avoid thrash just in case... But yeah, this is likely just totally dead. |
#1140