Skip to content

Fixed inconsistency in forwarding params in Context.forward#1840

Merged
davidism merged 1 commit intopallets:masterfrom
MLH-Fellowship:context-forward
Apr 13, 2021
Merged

Fixed inconsistency in forwarding params in Context.forward#1840
davidism merged 1 commit intopallets:masterfrom
MLH-Fellowship:context-forward

Conversation

@Saif807380
Copy link
Copy Markdown
Contributor

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.

From the example mentioned in the issue, now a union of params from the context and sub-context will be forwarded.

For e.g. - If command_1 has options a and b and command_2 has options a and c, command_1 forwards to command_2, then command_2 will receive a, b and c as kwargs.

I was unsure whether or not to remove the unused options, so I've kept them for now.

Checklist:

  • Add tests that demonstrate the correct behaviour of the change. Tests should fail without the change.
  • Add or update relevant docs, in the docs folder and in code.
  • Add an entry in CHANGES.rst summarizing the change and linking to the issue.
  • Add .. versionchanged:: entries in any relevant code docs.
  • Run pre-commit hooks and fix any issues.
  • Run pytest and tox, no tests failed.

@davidism davidism added this to the 8.0.0 milestone Apr 12, 2021
@davidism
Copy link
Copy Markdown
Member

davidism commented Apr 13, 2021

It seems a little weird to treat all kwargs as params, since params is supposed to hold the parsed values from @argument and @option. Another option is to separate into ctx.params and ctx.kwargs to track the decorated parameters vs everything the command was called with. I think it's fine as is for now, I'm not entirely sure the kwargs idea is any more "correct" in terms of the intentions of invoke, forward, and params.

@davidism davidism merged commit e3594e7 into pallets:master Apr 13, 2021
@davidism davidism deleted the context-forward branch April 13, 2021 02:36
@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 this pull request may close these issues.

context.forward inconsistently removes undeclared options

2 participants