Skip to content
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

context.forward inconsistently removes undeclared options #1568

Closed
thejohnfreeman opened this issue May 29, 2020 · 4 comments · Fixed by #1840
Closed

context.forward inconsistently removes undeclared options #1568

thejohnfreeman opened this issue May 29, 2020 · 4 comments · Fixed by #1840
Milestone

Comments

@thejohnfreeman
Copy link

thejohnfreeman commented May 29, 2020

Expected Behavior

Three commands. The first takes a subset of the options that the last two take. Each prints the arguments they were called with. All but the first call the one before it via context.forward.

import click
from pprint import pprint

@click.group()
def main():
    pass

@main.command()
@click.option('-a')
def first(**kwargs):
    print('first')
    pprint(kwargs)

@main.command()
@click.option('-a')
@click.option('-b')
@click.pass_context
def second(context, **kwargs):
    print('second')
    pprint(kwargs)
    context.forward(first)

@main.command()
@click.option('-a')
@click.option('-b')
@click.pass_context
def third(context, **kwargs):
    print('third')
    pprint(kwargs)
    context.forward(second)

main()

Here's the behavior I expect from each command. Notably, first is called with the same arguments, its declared options, each time.

$ python main.py first
first
{'a': None}
$ python main.py second
second
{'a': None, 'b': None}
first
{'a': None}
$ python main.py third
third
{'a': None, 'b': None}
second
{'a': None, 'b': None}
first
{'a': None}

Actual Behavior

$ python main.py first
first
{'a': None}
$ python main.py second
second
{'a': None, 'b': None}
first
{'a': None, 'b': None}     # 'first' is called with 'b' here
$ python main.py third
third
{'a': None, 'b': None}
second
{'a': None, 'b': None}
first
{'a': None}                # but not here

Environment

  • Python version: 3.7.5
  • Click version: 7.1.2
@0x1za
Copy link
Contributor

0x1za commented Jun 30, 2020

@thejohnfreeman I am looking into this. Doing preliminary investigations on why this is happening.

@parabolize
Copy link

parabolize commented Aug 31, 2020

Context.forward uses Context.params to fill in the options and passes the kwargs to Context.invoke which creates a new Context for the sub command but doesn't pass the params to the new Context nor does it remove unused kwargs. I'm not sure it should remove unused kwargs.

Below I've expanded your example to show the current behavior and a patch that would provide the behavior you are expecting:

import click


@click.group()
def main():
    pass


@main.command()
@click.option('-a')
@click.pass_context
def first(context, **kwargs):
    pp_kwargs('first', kwargs, context)


@main.command()
@click.option('-a')
@click.option('-b')
@click.pass_context
def second(context, **kwargs):
    pp_kwargs('second', kwargs, context)
    context.forward(first)


@main.command()
@click.option('-a')
@click.option('-b')
@click.pass_context
def third(context, **kwargs):
    pp_kwargs('third', kwargs, context)
    context.forward(second)


def pp_kwargs(f, kw, c):
    click.echo(f'\nin {f} callback: {kw}')
    click.echo(f'have context for {c.command}')
    click.echo(f'Context.params:{c.params}')

Current behavior:

% forward third -a foo -b bar

in third callback: {'a': 'foo', 'b': 'bar'}
have context for <Command third>
Context.params:{'a': 'foo', 'b': 'bar'}

in second callback: {'a': 'foo', 'b': 'bar'}
have context for <Command second>
Context.params:{}

in first callback: {'a': None}
have context for <Command first>
Context.params:{}

A possible patch:

diff --git a/src/click/core.py b/src/click/core.py
index 3bbc120..2accfbe 100644
--- a/src/click/core.py
+++ b/src/click/core.py
@@ -694,10 +694,19 @@ class Context:

             ctx = self._make_sub_context(other_cmd)

+            names = [p.name for p in other_cmd.params]
+            clean_kwargs = {}
+            for k, v in kwargs.items():
+                if k in names:
+                    clean_kwargs[k] = v
+            kwargs = clean_kwargs
+
             for param in other_cmd.params:
                 if param.name not in kwargs and param.expose_value:
                     kwargs[param.name] = param.get_default(ctx)

+            ctx.params.update(kwargs)
+
         args = args[2:]
         with augment_usage_errors(self):
             with ctx:

New output:

% forward third -a foo -b bar

in third callback: {'a': 'foo', 'b': 'bar'}
have context for <Command third>
Context.params:{'a': 'foo', 'b': 'bar'}

in second callback: {'a': 'foo', 'b': 'bar'}
have context for <Command second>
Context.params:{'a': 'foo', 'b': 'bar'}

in first callback: {'a': 'foo'}
have context for <Command first>
Context.params:{'a': 'foo'}

@thejohnfreeman
Copy link
Author

thejohnfreeman commented Sep 4, 2020

I've got a similar bug again. Might just be another instance of the same bug. It's been awhile since I revisited this code, stuck as I was on this bug with no workaround last time.

Three commands: project, configure, build. Each calls the one "before" it via context.forward. Thus, each takes the options that the first one takes. This time, that option is a "feature switch" in the Click documentation parlance: three options targeting the same parameter, but each option sets a different value for that parameter. Each command just prints the value for that parameter after forwarding.

import click
from toolz.functoolz import compose

flavor_option = compose(
    click.option(
        '--debug',
        'flavor',
        flag_value='debug',
        help='Shorthand for --flavor debug.',
    ),
    click.option(
        '--release',
        'flavor',
        flag_value='release',
        help='Shorthand for --flavor release.',
    ),
    # This must be composed last
    # because `flag_value` sets the default to `False`.
    click.option(
        '--flavor',
        'flavor',
        default='debug',
        help='The configuration flavor.',
    ),
)

@click.group()
def main():
    pass

@main.command()
@flavor_option
def project(flavor):
    print(flavor, 'project')

@main.command()
@flavor_option
@click.pass_context
def configure(context, flavor):
    context.forward(project)
    print(flavor, 'configure')

@main.command()
@flavor_option
@click.pass_context
def build(context, flavor):
    context.forward(configure)
    print(flavor, 'build')

if __name__ == '__main__':
    main()
$ poetry run python test.py project
debug project
$ poetry run python test.py configure
debug project
debug configure
$ poetry run python test.py build
False project                       # I was expecting 'debug' here
debug configure
debug build

@parabolize
Copy link

parabolize commented Sep 5, 2020

Its another instance of the same bug. You might try to work around it by moving the options up to the group and handle the shared logic outside the callback functions:

import inspect

import attr
import click


@click.group()
@click.option('--debug', 'flavor',
              flag_value='debug',
              help='Shorthand for --flavor debug')
@click.option('--release', 'flavor',
              flag_value='release',
              help='Shorthand for --flavor release')
@click.option('--flavor', 'flavor',
              default='debug',
              help='The configuration flavor')
@click.pass_context
def main(context, flavor):
    context.obj = Handler(flavor=flavor)


@main.command()
@click.pass_obj
def project(obj):
    click.echo(obj.project())


@main.command()
@click.pass_obj
def configure(obj):
    click.echo(obj.configure())


@main.command()
@click.pass_obj
def build(obj):
    click.echo(obj.build())


@attr.s
class Handler():
    flavor = attr.ib()

    def format(self, frame):
        method_name = frame.f_code.co_name
        return f'{method_name}: {self.flavor}'

    def project(self):
        return self.format(inspect.currentframe())

    def configure(self):
        rv = [self.format(inspect.currentframe()),
              self.project()]
        return '\n'.join(rv)

    def build(self):
        rv = [self.format(inspect.currentframe()),
              self.configure()]
        return '\n'.join(rv)
% test --flavor=blah build
build: blah
configure: blah
project: blah

% test build
build: debug
configure: debug
project: debug

% test project
project: debug

@davidism davidism added this to the 8.0.0 milestone Apr 12, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants