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

Centralized plugin loading that behaves just like click. #3

Merged
merged 10 commits into from
May 18, 2015
Merged

Conversation

geowurster
Copy link
Contributor

For #2

@sgillies I figured out how to simplify the API to the point where it behaves just like click normally does. I think this is actually something click should be able to handle natively so we should consider submitting a pull request, although their release cycle is kinda slow we will probably have to use the cligj implementation for a while. I'll add unittests after you review.

How does it work?

There's a full explanation of how this works in cligj.plugins.__doc__ but in general it works by subclassing and intercepting click components, executing the entry point loading logic, attaching commands, broken commands, or groups, and then calling the equivalent click command. Here's a CLI interface that has a main group, a sub command, and loaded plugins:

import click
import cligj.plugins
from pkg_resources import iter_entry_points

@cligj.plugins.group(plugins=iter_entry_points('module.commands'))
def cli():

    """A CLI application."""

    pass

@cli.command()
@click.argument('val')
def printer(val):

    """Print a value."""

   click.echo(val)

Extra gains

By doing this with decorators and subclassing click.Group(), commands and plugins are completely modular and can be moved from group to group in any click based application. Here's the same example from above but with all of the rio commands attached to a plugins sub-group that also has its own command:

from pkg_resources import iter_entry_points
import click
import cligj.plugins

# If any sub-groups are going to register plugins then the parent group has to come from cligj.
# This can also be done with `click.group(cls=cligj.plugins.Group())`.
# We don't want any plugins on this main group though, only the `plugins` group.
@cligj.plugins.group()
def cli():

    """A CLI application."""

    pass

@cli.command()
@click.argument('val')
def printer(val):

    """Print a value."""

   click.echo(val)

# The group returned here is the modified `cligj.plugins.Group()`
@cli.group(plugins=iter_entry_points('rasterio.rio_commands'))
def plugins():

    """rio commands"""
    pass

@plugins.command()
@click.argument('val')
def shout(val):

    """
    Print a value loudly.
    """

    click.echo(val.upper())

fio & rio changes

I have a version of Fiona on a branch that uses this plugin architecture but I haven't fully looked around to see if there are additional changes that need to happen, but the tests pass, the few commands I tested worked, and breaking plugins works properly.

  1. Attaching commands to the main cli() on setup causes circular imports so all commands must be decorated as click.command().
  2. Anything in main.py that is imported into one of the command files must be moved elsewhere. I dumped them in a helper.py in the branch but that may not be the best location.
  3. main.py should exist only to provide an isolated entry point to the main cli group.
  4. cli.py should be where all commands are aggregated for entry point access.

Actions

  • Right now all the CLI commands are registered as entry points in rasterio.rio_commands and users are instructed to register their plugins in the same place. We will likely be saved some headaches later if rasterio.rio_commands is reserved for the official rio commands and users are instructed to register their plugins at rasterio.rio_plugins. cligj.plugins.group() should be changed to take a string or tuple in that case.
  • I think using the same syntax and object names as click is good, but it is unfortunate that cligj and click are so typographically similar. I think referencing the cligj objects with cligj.plugins may help.
  • Do cli.py and main.py have an intended use? Since the individual click commands can't be attached to the main group maybe we want to have a commands.py where we aggregate?
  • Where should helper functions live that are currently in main.py? In the branch I just created a helper.py file to get them out of the way.

@sgillies
Copy link
Contributor

@geowurster 👍 on this design.

Yes, it would be good to prevent user plugin commands from colliding with the standard commands, so a new named entry point group makes sense. I'd prefer the CLI stay as flat as possible, ie, rio foo vs rio contrib foo.

Yeah, "cligj" was maybe too cute. Oh, well.

Originally, before I dove into Click, there was just a main.py. I left it there as I moved the Click-specific code to cli.py. I'm not aware of any other projects importing those modules (and nobody should). Let's consolidate or rename freely.

I can't think of anything better than helpers.py if it's a module of functions only imported from main.py.

@geowurster
Copy link
Contributor Author

@sgillies agreed. Here's the order of operations:

  1. Add unittests to this then merge.
  2. Release cligj v0.2.
  3. Implement in rasterio.
  4. Implement in Fiona.
  5. Update plugin examples to use rasterio.rio_plugins.
  6. Submit a PR to click that lets it handle this directly.

Sound good? I can tackle all these except for the release and I don't see any blocking issues or PR's.

@geowurster geowurster assigned geowurster and unassigned sgillies May 14, 2015
@sgillies
Copy link
Contributor

@geowurster Good plan. I'll create the issues for Fiona (milestone 1.6) and Rasterio (1.0).

Ping me when it's merge ready and I'll do a last review and merge. I'll ask the same of you on my PRs. Okay?

@geowurster
Copy link
Contributor Author

Yep, absolutely.

@coveralls
Copy link

Coverage Status

Coverage decreased (-28.07%) to 71.93% when pulling bd9bbb3 on plugit into 7928e4a on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 46d6b0e on plugit into 7928e4a on master.

…a python module, which is required for manually registering and testing plugins.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling a32ed1e on plugit into 7928e4a on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling ab253c2 on plugit into 7928e4a on master.

@geowurster
Copy link
Contributor Author

@sgillies ready to go.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling f340bec on plugit into 7928e4a on master.

sgillies added a commit that referenced this pull request May 18, 2015
Centralized plugin loading that behaves just like click.
@sgillies sgillies merged commit 5a48405 into master May 18, 2015
@sgillies
Copy link
Contributor

👍

@sgillies sgillies added this to the 0.4 milestone May 28, 2015
@geowurster geowurster deleted the plugit branch June 28, 2023 02:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants