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

Allow adding dynamic parameters to every Context #2784

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

ofek
Copy link
Contributor

@ofek ofek commented Sep 28, 2024

This adds a new dynamic_params property to the Context object which the Command object will use in its get_params method.

fixes #2783

  • Add tests that demonstrate the correct behavior 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.

@davidism
Copy link
Member

I'm currently rewriting the parser, so I won't be able to add this until I understand how it will interact with that, which will be some time.

@davidism
Copy link
Member

I'm not sure I like this, since it moves parameter definitions out of the command itself. We already support dynamic commands, and that doesn't require storing them in the context.

@ofek
Copy link
Contributor Author

ofek commented Sep 28, 2024

The issue is dynamic parameters rather than commands themselves. Here is an example that I'm about to use at work using this PR implementation:

def resolve_environment(ctx: DynamicContext, param: click.Option, value: str) -> str:  # noqa: ARG001
    from msgspec_click import generate_options

    dev_env_class = get_dev_env(value)
    ctx.params['env_class'] = dev_env_class
    ctx.dynamic_params.extend(generate_options(dev_env_class.config_class()))
    return value

@click.command(short_help='Start a developer environment', context_settings={'ignore_unknown_options': True})
@click.option(
    '--type',
    '-t',
    'env_type',
    type=click.Choice(AVAILABLE_DEV_ENVS),
    default=DEFAULT_DEV_ENV,
    show_default=True,
    is_eager=True,
    callback=resolve_environment,
    help='The type of developer environment',
)
@click.pass_context
def cmd(ctx: click.Context, env_type: str, **kwargs: Any) -> None:
    import msgspec

    env_class: type[DeveloperEnvironmentInterface] = ctx.params['env_class']
    config = msgspec.convert(kwargs, env_class.config_class())
    env = env_class(
        name=env_type,
        storage_dirs=app.config.storage,
        config=config,
    )

@ofek
Copy link
Contributor Author

ofek commented Sep 28, 2024

I would be open to anything that allows me to make dynamic options per execution/context rather than per command!

edit: without the use of a synchronization primitive like threading.Lock 😉

@ofek
Copy link
Contributor Author

ofek commented Sep 28, 2024

it moves parameter definitions out of the command itself

For this point in particular, I was trying to copy the code style by adding a parameter which is saved as a property of the instance. I actually don't see a use case for passing in parameters and would much prefer only the property without modifying the signature of Context. Would you like me to do that?

@davidism
Copy link
Member

davidism commented Sep 28, 2024

The issue is dynamic parameters rather than commands themselves.

I know. With dynamic commands, you can override what commands are available within a group during execution, without special support in the the context and without modifying the global definition of the group. I want dynamic parameters to behave similarly.

@ofek
Copy link
Contributor Author

ofek commented Sep 28, 2024

I don't have enough knowledge of the code to understand the paradigm you desire. Could you perhaps show an example?

@ofek
Copy link
Contributor Author

ofek commented Oct 18, 2024

Edit: tests are now fixed and I added a new classmethod to Command that is required for this use case

@ofek
Copy link
Contributor Author

ofek commented Oct 19, 2024

The docs build error appears unrelated and I'm not sure how to fix that.

@Rowlando13
Copy link
Collaborator

@ofek Thanks for submitting this. It will be a while before we are able to tend to this. We are currently focusing on getting issues cleaned up and patching bugs. So I am going to revert this to draft.

@Rowlando13 Rowlando13 marked this pull request as draft December 8, 2024 06:30
@ofek
Copy link
Contributor Author

ofek commented Dec 8, 2024

Should I rebase to resolve conflicts or is it not worth it currently?

@Rowlando13
Copy link
Collaborator

I would say not worth the effort with where we are now. But if you would like to help with triaging issues, we would love the help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow attaching dynamic parameters to contexts
3 participants