-
Notifications
You must be signed in to change notification settings - Fork 259
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
If command matches non-installed plugin, offer install #1813
Conversation
Signed-off-by: itowlson <ivan.towlson@fermyon.com>
warn_unsupported_version(&manifest, SPIN_VERSION, override_compatibility_check) | ||
{ | ||
eprintln!("{e}"); | ||
// TODO: consider running the update checked? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, that could be handy. But potentially also another spot to check in with user 'no plugin manifest is found for foo, shall we update your local plugins list'? Though I think just proceeding seems like a sane default eg 'no plugin manifest is found for foo, pulling the latest available plugins from fermyon/spin-plugins...' (or other).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks for breaking this code out more. One nit question
if offer_install(plugin_name)? { | ||
let plugin_installer = installer_for(plugin_name); | ||
plugin_installer.run().await?; | ||
eprintln!(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed?
eprintln!(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for this was to create a visual separation between the "would you like to install it" prompt and the plugin output. Without the blank line, I felt they ran together rather. But this is very subjective for sure.
When the user invokes a command which is not recognised, but does match a plugin, this feature offers to install that plugin:
(I am now wondering if I should print the description as part of the prompt.)
Conceptually, this PR just adds another case to the "not found" match arm in
execute_external_subcommand
- there's really only a few lines of new code. But that function was getting so gnarly that I chose to break out the "make sure the plugin is there" stuff into a separate function... which was still gnarly, and anyway long story short here we are with a diff the length of your arm and yep, even now one of the functions is not free of the gnarl.