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.

click/docs/parameters.rst

Lines 93 to 95 in 2c622ee

	If all names for a parameter contain dashes, the internal name is generated
	automatically by taking the longest argument and converting all dashes to
	underscores.

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:

click/docs/options.rst

Lines 41 to 43 in b1b8c48

	:data:`STRING`. Unless a name is explicitly specified, the name of the
	parameter is the first long option defined; otherwise the first short one is
	used. By default, options are not required, however to make an option required,

vs parameters:

click/docs/parameters.rst

Lines 93 to 95 in 2c622ee

	If all names for a parameter contain dashes, the internal name is generated
	automatically by taking the longest argument and converting all dashes to
	underscores.

[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.

[altendky]

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

[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.

[altendky]

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

[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:

click/docs/options.rst

Lines 19 to 20 in df0e37d

	:data:`STRING`. By default, the name of the parameter is the first long
	option defined; otherwise the first short one is used.

parameters:

click/docs/parameters.rst

Lines 83 to 88 in df0e37d

	If a parameter is not given a name without dashes, a name is generated
	automatically by taking the longest argument and converting all dashes to
	underscores. For an option with ``('-f', '--foo-bar')``, the parameter
	name is `foo_bar`. For an option with ``('-x',)``, the parameter is `x`.
	For an option with ``('-f', '--filename', 'dest')``, the parameter is
	called `dest`.

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.

altendky@bcd70a3

@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).

[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.

[altendky]

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

[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

[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...

• 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 #1420 (comment) 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.