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

Add optional interactive shell through click-repl #351

Closed
wants to merge 1 commit into from

Conversation

barskern
Copy link

By installing click-repl0 the user gets an interactive shell with
tab completion enabled.

By installing `click-repl`[0] the user gets an interactive shell with
tab completion enabled.

[0]: https://github.com/untitaker/click-repl
@jmaupetit
Copy link
Contributor

Thank you for this @barskern

🤔 I have the feeling that it must be a plugin, and requires therefore that we implement a plugin architecture.

@barskern
Copy link
Author

Hmm I not sure what you mean. This PR doesn't add anything really besides the fact that it will add a repl command if the user already has click-repl installed.

@jmaupetit
Copy link
Contributor

You've added an extra dependency and its integration is hard coded in the core part of Watson. I think it's not a good architecture, and that we should add plugins support for such feature.

@barskern
Copy link
Author

barskern commented Jan 23, 2020 via email

@jmaupetit
Copy link
Contributor

technically I only added metadata about a optional dependency that is not installed with watson. Further if the dependency is not present, there is no change from the previous functionality.

Maybe you don't see it because you are the first one willing to add such pattern, but if we begin to do so, Watson will became a huge bloated mess with optional features and dependencies.

@barskern
Copy link
Author

barskern commented Jan 23, 2020 via email

@davidag
Copy link
Contributor

davidag commented Jan 26, 2020

This PR seems very similar to the one where did-you-mean support was added (#318), except in that case the dependency was not optional.

The point to me is if this feature makes sense for Watson users, which I'm not quite sure (I don't see myself using it).

@barskern
Copy link
Author

barskern commented Jan 26, 2020

Yeah @davidag, it might not fit that well into everyone's workflow, however personally I find it very handy. I have a script called watson-repl with the following contents:

#!/usr/bin/env bash
watson report -G
echo "---"
watson status
watson repl

And then I have a bind in my i3 config which launches the script in a floating terminal window:

bindsym $mod+Shift+w exec $term_wd --title "watson" --class "floater" -e watson-repl

When the script launches I get a terminal with pre-printed status information and a repl where I don't have to start every command with watson.

Copy link
Collaborator

@k4nar k4nar left a comment

Choose a reason for hiding this comment

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

I second @jmaupetit here.

Adding an optional dependency & 3 lines of conditional code seems harmless, but it means that once this is released, users will expect this option to be supported. We would now depend on the fact that click-repl is maintained & keep working in the future. It doesn't seem actively maintained nor used, so it might not be the best bet.

I like your example though, it seems very convenient. If you wanted to publish a simple package (watson-repl ;) ) to enable this, we could advertise it in the doc or the readme, as we did with past projects.

@barskern
Copy link
Author

Yeah, I understand your reasoning.

My script is very convenient, and I would be happy to make a package for it, however I am not sure of how to do it. Here are some of the options I could consider:

  • Non-automated "simple" fork

Is a lot of work to keep up-to-date.

  • Automated fork

Automatically pull latest changes from upstream through a webhook and merge into package. Potentially low maintance however probably takes some work to setup correctly.

  • Simple patch using diff and apply before build

Will probably quickly get out-of-date and hence the patch will not apply smoothly.

Any better solutions or tips for my proposed solutions?

@k4nar
Copy link
Collaborator

k4nar commented Jan 30, 2020

Maybe you could package a python script which would include watson, setup the repl, and expose an entrypoint named watson-repl.

Or, as @jmaupetit suggested, we could try to setup a plugin system.

@barskern barskern closed this Jun 2, 2021
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