Skip to content

A way of reusing the elsa cli #45

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

Merged
merged 6 commits into from
Mar 25, 2018
Merged

A way of reusing the elsa cli #45

merged 6 commits into from
Mar 25, 2018

Conversation

mikicz
Copy link
Member

@mikicz mikicz commented Mar 25, 2018

I want to add a command to naucse, however, elsa invokes the cli right away and I couldn't way a way to "subclass" the cli other than this one.

The following is an example from naucse, when this function is called, all three commands from elsa and the new_command are available from command line.

import elsa
import click

def cli(app, *, base_url=None, freezer=None):
    elsa_group = elsa.cli(app, base_url=base_url, freezer=freezer, invoke_cli=False)

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

    @naucse.command()
    def new_command():
        """ Some new command. """

    cli = click.CommandCollection(sources=[naucse, elsa_group])

    return cli()

@hroncok
Copy link
Member

hroncok commented Mar 25, 2018

This is hack-ish, I don't like the API. But having a different API would be backwards incompatible :(
In reality, we should do #39 properly instead. However #39 is stalled for more than a half of year, so I'm probably OK with this. Please add a test tough and update the docstring, which currently lies a bit I guess (I mean before this PR anyway).

@mikicz
Copy link
Member Author

mikicz commented Mar 25, 2018

I updated the docstring and added a test, that tests adding a custom command if invoke_cli is set to False. Is it enough? I'm not sure how to test it more thoroughly...

I get that this isn't the ideal solution, however, I took a look at #39 and the problem is still there, the Elsa.cli method invokes the cli and doesn't enable to "subclass" the cli anyway.

Copy link
Member

@hroncok hroncok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will squash this.

@hroncok hroncok merged commit 5bae96c into pyvec:master Mar 25, 2018
@hroncok
Copy link
Member

hroncok commented Mar 25, 2018

Thanks.

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

Successfully merging this pull request may close these issues.

2 participants