Click: Longest option argument not used as internal/variable name

[altendky]

It seems that with 7.0 the longest option argument isn't being used as the internal and variable name.

https://github.com/pallets/click/blob/2c622eed402c64e1645db3904c2410f6c05fbf19/docs/parameters.rst#L93-L95

the internal name is generated automatically by taking the longest argument

import click
import click.testing


print(click.__version__)

@click.command()
@click.option(
    '--temp',
    '--temporary',
    'temporary',
)
def good(temporary):
    pass


@click.command()
@click.option(
    '--temp',
    '--temporary',
)
def bad(temporary):
    pass


def click_runner(command):
    print(command.name)
    runner = click.testing.CliRunner()
    result = runner.invoke(
        command,
        [],
        catch_exceptions=False,
    )
    print(result.output)

click_runner(command=good)
click_runner(command=bad)

https://repl.it/@altendky/click-67-unexpected-keyword-argument-1

6.7
good

bad

https://repl.it/@altendky/click-70-unexpected-keyword-argument-1

7.0
good

bad
Traceback (most recent call last):
  File "python", line 37, in <module>
  File "python", line 32, in click_runner
TypeError: bad() got an unexpected keyword argument 'temp'

https://repl.it/@altendky/click-2e856a5-unexpected-keyword-argument

7.0
good

bad
Traceback (most recent call last):
  File "python", line 37, in <module>
  File "python", line 32, in click_runner
TypeError: bad() got an unexpected keyword argument 'temp'

[altendky]

Seems to relate to #793/#794 where a different part of the docs were referenced. So I guess the question is which one do we want?

options:
https://github.com/pallets/click/blob/b1b8c48345d8accc4dea262b8a41ac5ac1de3af2/docs/options.rst#L41-L43

vs parameters:
https://github.com/pallets/click/blob/2c622eed402c64e1645db3904c2410f6c05fbf19/docs/parameters.rst#L93-L95

[amiryal]

@altendky I would suggest to revert to the old (pre #794) behaviour to fix the regression, and align the documentation with actual implementation instead.

[altendky]

@davidism, I could presumably put together a PR... but this seems more like a policy decision than just a fix. If whoever would make that decision can let me know I can try to implement it (code/tests/docs).

[davidism]

Thanks! Seems better to revert to the old behavior and update the docs.

[altendky]

@davidism should the PR be against 7.x or master? Am I supposed to modify CHANGES.rst?

[davidism]

It should be against 7.x. You can add a change entry that points to the pr now that it's submitted.

[kiwi0fruit]

This breaks backward compatibility. Click used to use the last --arg (as I thought... but it holds in my case) - now it uses the first (or maybe it's the first longest).

[altendky]

@kiwi0fruit are you referring to #1149 not properly reverting functionality?

[kiwi0fruit]

• @click.option('-f', '-r', '--from', '--read') should pick read not from.
• @click.option('-w', '-t', '--write', '--to') should pick to not write.

Otherwise click broke my program. And broke backward compatibility. Later I would be able to provide you with versions that work and broke.

UPD: works on click v.6.7
UPD2: breaks on click v7.0

[altendky]

I'm presently setting up the tests against 6.7 as the reference since the point is to revert to that functionality, but with consistent docs.

[altendky]

It's kind of tempting to deprecate this 'feature' and just say 'if you have multiple of the longest type of parameter, specify the name'. Or maybe provide a marker so you don't have to repeat. Maybe '+--the-default' or somesuch? I dunno, explicit over implicit and all. This just seems like a bit of a fiddly mess with little benefit.

[kiwi0fruit]

Why not have it backward compatible?

[kiwi0fruit]

The 6.7 behavior was not picking the longest. It picked to instead of write.

[altendky]

Sorry, make it backwards compatible but fix the situation by deprecating the implicit order-based selection.

[kiwi0fruit]

I thought that he picked the last in order given. What makes you think that it's related to argument length? (if we speak about 6.7)

[kiwi0fruit]

Why deprecate order based selection when it works?

[davidism]

@kiwi0fruit please stop having this discussion. 6 -> 7 was a major version, it's not required to maintain compatibility. That said, we've already acknowledged that we want to adjust the behavior. If you want to provide constructive feedback, look into comparing the code and behaviors of the two versions with actual examples.

[altendky]

The docs. Though admittedly above I believe I linked latest docs, not 6.7 docs. But, even 6.7 docs were inconsistent.

options:
https://github.com/pallets/click/blob/df0e37dd890d36fc997986ae6d2b6c255f3ed1dc/docs/options.rst#L19-L20

parameters:
https://github.com/pallets/click/blob/df0e37dd890d36fc997986ae6d2b6c255f3ed1dc/docs/parameters.rst#L83-L88

I also checked out 6.7 and pulled a branch to validate tests against. The tests in #1149 did not pass so I adjusted them as follows so we could have an actual 6.7 reference. I am happy to add more cases to that branch so as to keep them all together.

https://github.com/altendky/click/commit/bcd70a3702dffdca92bd1433b69de87a25998bbb

@pytest.mark.parametrize('option_args,expected', [
    (['--aggressive', '--all', '-a'], 'all'),
    (['--first', '--second', '--third', '-a', '-b', '-c'], 'third'),
    (['--apple', '--banana', '--cantaloupe', '-a', '-b', '-c'], 'cantaloupe'),
    (['--cantaloupe', '--banana', '--apple', '-c', '-b', '-a'], 'apple'),
    (['-a', '-b', '-c'], 'c'),
    (['-c', '-b', '-a'], 'a'),
    (['-a', '--apple', '-b', '--banana', '-c', '--cantaloupe'], 'cantaloupe'),
    (['-c', '-a', '--cantaloupe', '-b', '--banana', '--apple'], 'apple'),
    (['--from', '-f', '_from'], '_from'),
    (['--return', '-r', '_ret'], '_ret'),
    (['-f', '-r', '--from', '--read'], 'read'),
    (['-w', '-t', '--write', '--to'], 'to'),
])

Sure, 6->7 is a major version but I wouldn't want to change just because. But, I think that the interface is bad if nothing else because we seem to have three definitions that all conflict (options: "first long option", parameters: "longest argument", code: picks last long). How about we make the rule super simple and just require people to be explicit in the face of ambiguity. shrug I would think the + prefix or similar wouldn't be particularly onerous.

Anyways, I'll hold off on making any changes to the PR until we decide what behavior we really want. At this point we may have to consider that 6->7 breaking may not be preferred but was 'technically acceptable' while reverting the functionality in 7->7.1 (or whatever this might end up in) would be might be considered a regression of sorts. I dunno. I just put an explicit name in our code so we could stop caring what happens here (as far as our code working).

[kiwi0fruit]

Oh, I see. So this mistake was intentional. OK. There is nothing I can add on this.

[altendky]

@kiwi0fruit, I never meant to advocate changing the interface from 6.7. I just made the mistake of looking at the docs rather than the code. At which point we could even argue that 6.7 actual behavior was a bug since it didn't match either piece of documentation. But still, when you are stuck with 3 inconsistent 'policies' and now have new functionality in a new major version... Let's at least consider where we should end up. Then how to get there with the least surprise and frustration.

Side note, this made me start looking into actual CI. Not just build on each commit, but integrating latest on each build. Not that this change was that horrible, it just triggered me to actually act and try to setup CI with Tox, which was reasonably painful. I added a few extra jobs to do latest deps, latest --pre deps, and vcs deps. Hopefully this will help us catch these things prior to release. Though having a prerelease on PyPI could also help encourage people to use things before a full release.

[kiwi0fruit]

If you need to unify and break backward compatibility it obviously better unify to most common use-case (what interface users of click library use most often?). I'm not sure what it is so it needs investigation.

[altendky]

@kiwi0fruit, you dislike requiring an explicit decision of the developer?

[kiwi0fruit]

@altendky I don't really care. I don't have a packege that would break. I am a passer by that saw people breaking backward compatibility and warned them about it as it almost always a mistake. // Actually I have a package but nobody uses it so it doesn't matter.

[altendky]

@kiwi0fruit, consider that this ticket exists exactly because we observed the change...

[altendky]

I haven't thought it through a lot but just so we have something (more) concrete to consider...

• Immediately
  
  
  
  • Indicate old interface as deprecated and raise a warning for any use of it
  
  
  • Implement a new explicit interface
  
  
  • + prefix to specify the parameter which should be processed to create the variable name
  
  
  • If no argument starts with a +, retain 6.7 functionality
  
  
  • Update docs to reflect new explicit rules
    
    
    
    
    
    • This includes removing reference to old rules that didn't even match the code
    
    
    
  
  
  • If any argument starts with + use the new strict and explicit rules
  
  
  • New strict and explicit rules
    
    
    
    
    
    • If other than a single argument starts with + raises an exception
    
    
    
    • Old-style explicit name specification (thename without any leading -'s) must be adjusted to start with a +. Without is an error.
    
    
    
  
  
• Later (I dunno, two years? Click 8.0?)
  
  
  
  • Strictly require new explicit rules
  
  

[kiwi0fruit]

By the way. If the question is only in mappings of options to arguments (and there are three different ways of such mappings unfortunately) then they all can be unified by intelligent guesser that returns solution when there is only one solution and returns error when there are more than one solution or no solution.

[kiwi0fruit]

Or may be it returns error also when any one of python function arguments is present in more than one list of CLI options (or not in any). If such behavior would be slow then it should be fallback behavior.

[altendky]

I considered there being possible improvements to automatic selection. It didn't seem like they would provide much value over just checking for an explicit choice by the developer. Explicit has the upsides of simple everything for everyone. Nobody has to remember anything beyond the explicit indicator.

[altendky]

And a single extra character seems like a minimal burden.

[altendky]

So we have two things to address. How do we handle the documentation/code disagreement right now and where do we want to go in the future.

Right now we should do one of...

• [x] Correct docs to match 7.0 code behavior
• [ ] Change code and docs to match 6.7 code behavior

Later, none or more of...

• [ ] Require explicit variable name specification
• [ ] Provide a less repetitive way to specify the name explicitly

And maybe also consider doing prereleases in hopes to catch stuff like this before release next time. Maybe, I dunno.

(@sirosen)

[davidism]

Well, totally forgot this discussion happened, then did some digging here https://github.com/pallets/click/issues/1420#issuecomment-595903444 when it was brought up again. Based on that, my decision is that the change to use the first argument, preferring double dash over single dash, was intentional, and the docs should be updated to be clear about the rules as they currently work.