Skip to content

List plugins in help #1560

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 1 commit into from
Jun 13, 2023
Merged

Conversation

itowlson
Copy link
Collaborator

@itowlson itowlson commented Jun 8, 2023

This is an experiment to see if people like this experience or not. Here's what it looks like on the proverbial my machine:

$ spin
spin 1.3.0-pre0 (b6a752f 2023-06-08)
The Spin CLI

USAGE:
    spin <SUBCOMMAND>

OPTIONS:
    -h, --help       Print help information
    -V, --version    Print version information

SUBCOMMANDS:
    add          Scaffold a new component into an existing application
    build        Build the Spin application
    cloud        Commands for publishing applications to the Fermyon Cloud.
    deploy       Package and upload an application to the Fermyon Platform
    help         Print this message or the help of the given subcommand(s)
    js2wasm      A plugin to convert js files to Spin compatible modules
    login        Log into to the Fermyon Cloud.
    new          Scaffold a new application based on a template
    pluginify    
    plugins      Install/uninstall Spin plugins
    py2wasm      A plugin to convert Python applications to Spin compatible modules
    registry     Commands for working with OCI registries to distribute applications
    templates    Commands for working with WebAssembly component templates
    timer        IT IS I THE GREAT OZ
    up           Start the Spin application
    watch        Build and run the Spin application, rebuilding and restarting it when files
                     change

At the moment, I expect there to be a bug when the cloud plugin is installed, since that's a legit command as well. I have a feeling I can solve this by steeples fingers abolishing the cloud command and just always injecting the help entry - because Kate has put all the magic into the external side rather than the command side, I think it will behave correctly even if no actual CloudCommand exists. But I will investigate that if we decide to go forward with this; one way or another it will be fixable.

@mikkelhegn
Copy link
Contributor

I think this is great.

One suggestion may be to call out more explicit which commands are part of a plugin. E.g.

    add        Scaffold a new component into an existing application
    build      Build the Spin application
    cloud      [plugin] Commands for publishing applications to the Fermyon Cloud.
    deploy     [plugin] Package and upload an application to the Fermyon Platform
    help       Print this message or the help of the given subcommand(s)
    js2wasm    [plugin] A plugin to convert js files to Spin compatible modules

That way if you are comparing screenshots in tutorials or somewhere else, it's clear if a comand is missing in your list, that it came from a plugin.

@kate-goldenring
Copy link
Contributor

IT IS I THE GREAT OZ 🤣

@itowlson
Copy link
Collaborator Author

itowlson commented Jun 8, 2023

@kate-goldenring I am really hoping I didn't leave that in the actual timer trigger sample.

@itowlson
Copy link
Collaborator Author

itowlson commented Jun 9, 2023

@mikkelhegn I agree identifying plugins would be good. I considered a couple of things:

  • Different sections in the list (e.g. SUBCOMMANDS: / PLUGINS:). The framework we're using doesn't yet allow that (and the issue for it has been open for years, so we should assume it never will).
  • Prepending [plugin] (or similar) to the description. My inner nerd dislikes the way this messes up the alignment of the description. From a less subjective point of view, it also gets in the way of the eye finding the actual start of the description.
    • I looked at if we could shift the built-in commands a bit right to make room for it but it didn't seem like that would be easy.
  • Appending an emoticon indicator to the name. This might look okay but I struggled to find an icon that had good clarity and contrast but didn't look misproportioned and drag the eye away from the content (see screenshot).
    • A more discreet alternative is to append a * but that may confuse people as to "why do some of my commands have asterisks on them." (See other screenshot.)

image

image

I'd very much welcome insight, ideas and feedback.

@mikkelhegn
Copy link
Contributor

Although not as satisfying as using an emoji, I think either of theses works. If we can add a line of text at the bottom. something like [p] subcommand from plugin or [*] subcommand from plugin.

Using [p] is probably less "scarry"?

image image

@itowlson
Copy link
Collaborator Author

Where this is now at:

The UI looks like this (@mikkelhegn let me know if you have a preference for the bracketed form):

image

The various combinations of plugins / flags / help options / etc. that I have tested all seem to work, though it would be great to have someone else trying to break it (and it might be good to merge it only after we cut 1.3, because if I have fouled up the entire command line then it would be good to have a bit of time to discover it).

The cloud command has been removed: spin cloud now goes directly to execute_external_subcommand. The external subcommands module provides a declarative way to define these 'magic' plugins via the PREDEFINED_EXTERNALS array.

Plugins are now considered for the "similar commands" prompt:

ivan@hecate:~/testing/pyfie$ spin clod
Error: 'clod' is not a known Spin command. See spin --help.

The most similar command is:
    cloud

ivan@hecate:~/testing/pyfie$ spin ps2wasm
Error: 'ps2wasm' is not a known Spin command. See spin --help.

The most similar commands are:
    js2wasm
    py2wasm

The commits still need squashing.

Feedback and complaints very much welcome.

@mikkelhegn
Copy link
Contributor

I think this looks great, and gets the job done 👍🏻

@mikkelhegn
Copy link
Contributor

Not sure what's going on here?

The cloud command is still highlighted, even though I uninstalled the plugin.

image

@kate-goldenring
Copy link
Contributor

@mikkelhegn what do you mean by highlighted? The fact that cloud is still asterixed despite not being installed? that is interesting

@kate-goldenring
Copy link
Contributor

@itowlson should deploy and login always be decorated with *?

@itowlson
Copy link
Collaborator Author

@mikkelhegn cloud is always shown, even if not installed; I believe this was the plan, and is why the CloudCommand currently exists. It is asterisked because it is delivered via a plugin. That the plugin isn't currently installed doesn't change the fact that it's delivered via a plugin!

That said, if the goal of the asterisks is more to highlight "places where your list may be different from other people's" rather than "is implemented via a plugin" then agree, we should take the asterisk off cloud, and we should not show it even when the plugin is installed (which probably means we need to rephrase the explanation of the asterisk).

@kate-goldenring That's a good question... this is probably an artifact of the way I implemented the asterisking system (which I guess is true of Mikkel's cloud as well). There is also a sense in which deploy and login are Spin commands with their own logic which make use of a plugin.

But yeah... I guess it comes back to "is the idea to show which commands are implemented via plugins or to show which commands may not appear in docs or on other machines." Let's define that, and maybe that will guide us on how to handle these cross-over cases.

@itowlson itowlson force-pushed the list-plugins-in-help branch from 992ab2e to 660c519 Compare June 12, 2023 23:55
@mikkelhegn
Copy link
Contributor

Thanks for the explanation @itowlson. I think the purpose should be "to show which commands are implemented via plugins".

I guess for plugins we allow to overwrite commands, is that true? If so, it would be great to be able to still have the asterisk mean the same - i.e., "On this machine the following commands are handled by plugins".

@kate-goldenring
Copy link
Contributor

I guess for plugins we allow to overwrite commands, is that true? If so, it would be great to be able to still have the asterisk mean the same - i.e., "On this machine the following commands are handled by plugins".

@mikkelhegn plugins cannot overwrite commands that are build into the Spin CLI. cloud, deploy and login are unique in that we know in spin they are aliases to a plugin

@kate-goldenring
Copy link
Contributor

I agree that the indicator should be "is implemented via a plugin". i am now torn on deploy and login. It would be nice to be able to list your plugins and see by name how they map to the cli and that is not the case for deploy and login

@itowlson itowlson requested a review from fibonacci1729 June 13, 2023 19:49
@itowlson
Copy link
Collaborator Author

Given Mikkel's comment and Kate being torn, can I propose we merge this as stands, with the option to refine it before (or after) 1.4? That would give people the opportunity to shake out any bugs in parallel with UI refinement; otherwise we risk going back and forth for ages (given that every round of conversation takes 24 hours because shakes puny fist at timezones), and finally landing it without time to bake.

If that is okay, it would be super helpful to get a review so we can start moving it forward.

Copy link
Contributor

@fibonacci1729 fibonacci1729 left a comment

Choose a reason for hiding this comment

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

LGTM! I agree with @itowlson that it's probably best to merge now and tease out the desired enhancements before 1.4

Copy link
Contributor

@kate-goldenring kate-goldenring left a comment

Choose a reason for hiding this comment

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

Very nifty use of adding commands to clap! LGTM

@itowlson itowlson force-pushed the list-plugins-in-help branch from 660c519 to 77d3d09 Compare June 13, 2023 21:11
@itowlson itowlson merged commit 164b656 into spinframework:main Jun 13, 2023
@itowlson itowlson mentioned this pull request Sep 22, 2023
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.

4 participants