fadeevab / cliclack

Beautiful, minimal, opinionated CLI prompts inspired by the Clack NPM package
MIT License
214 stars 15 forks source link

[improvement] add `required()` to `Select<T>` #78

Closed romancitodev closed 1 month ago

romancitodev commented 1 month ago

Sometimes, in some selection, perhaps you may not want select anything because you are in a skippable step.

for example I am writing a CLI tool to write commits in an easy way and some steps are skippable like this: image

Maybe you don't have a scope or you don't want to set any scope. Maybe these days I'll do a PR to add this.

fadeevab commented 1 month ago

@romancitodev You would probably better use the first item called like "(none)" or "(skip)". I'm unsure how to render a skippable list to avoid confusing a user.

romancitodev commented 1 month ago

That's could probably be a patch, not a solution ideally.

The thing is very simple, due to Rust error handling system, we can return a Result<Option<T>, Error> instead of simply an empty string or an arbitrary one.

And so to handle the "skip", we can set a combination of keys to submit the state of the prompt.

https://github.com/fadeevab/cliclack/blob/main/src/select.rs#L106

By the way, I'll try to implement it by my own and open a little PR adding that.

fadeevab commented 1 month ago

@romancitodev Hm, I don't really see what would be easier for user than hitting Enter on the 1st default item that says (none). Introducing key combinations is usually not that convenient. But overall, I'm open for more ideas around this case.

romancitodev commented 1 month ago

In this particular case, you only have to take one key combination, there is no confusion about other combinations because they don't exist.

I agree with you about making life easier for the user, but sometimes "simple" is not the right thing to do.

And I surround it with "" because in most cases simple is better, but taking an arbitrary value like (none) or anything else maybe can be worse for the developer experience, we are using Rust which is an amazing tool and if we can handle the use cases with things like Option it can be even more idiomatic and easy for everyone.

btw, it is your project so you decide how you want the project to grow.

fadeevab commented 1 month ago

@romancitodev Thanks, I understand you. Let's look at it from another perspective: how do you envision it being rendered?

romancitodev commented 1 month ago

Well, I thought of using a special trait called SkippablePrompt<T> which implements the same as PromptInteraction<T> but the difference is the interact*() methods.

These methods return a Result<Option<T>, Error> and the trait can be used in other structs replicating the business logic.

A second idea can be to play with type implementations as in this video: https://youtu.be/_ccDqRTx-JU and make components depend on some kind of states. (this approach even can be more idiomatic and can be more useful instead of duplicating code and traits.

The render logic must be the same as the PromptInteraction, i mean, nothing must change because we are just changing the type of returning, but for the user is the same thing.

A plus could be adding a little signature "( to skip)" aside the prompt header.

fadeevab commented 1 month ago

@romancitodev I mean this part: "A plus could be adding a little signature "(to skip)" aside the prompt header." How would it prompt the user that the list is "skippable". Not the code-wise yet. :)

romancitodev commented 1 month ago

my bad, Github didn't formatted well the signature.

(<combination> to skip)

where the combination could be Ctrl + S or anything else.

cylewitruk commented 1 month ago

Just a little chime-in with my $0.02...

I personally (as a user) think that the suggestion of the first item being "(none)" or similar is the better UX over fumbling with key combinations... not sure I've ever seen a skippable prompt with a key combination to skip tbh...

However I do think that this could indeed be provided by cliclack with a .with_skip_text() or something which steers the render and then an Option<T> is returned if the skip text is matched.

Just food for thought!

P.s. you could also have a look at my PR #72 which aims to add implicit Option<T> support for input, could probably take some inspiration from there.

fadeevab commented 1 month ago

Maybe, as an option, the following might be a way to go: skippable (or with_skip_text, whatever) introduces the item 0 with a text of another color, so it would make sense, and selecting those item makes it return Err(Error::Skipped). (So we don't break API compatibility).

It's a bit clunky but brings some specific API support and perhaps unique visual representation.

romancitodev commented 1 month ago

P.s. you could also have a look at my PR #72 which aims to add implicit Option<T> support for input, could probably take some inspiration from there.

@cylewitruk I started the PR without looking for any similar one but I'm glad to have a better reference that my code and I'm very excited to implement something like the PR you did.

romancitodev commented 1 month ago

Maybe, as an option, the following might be a way to go: skippable (or with_skip_text, whatever) introduces the item 0 with a text of another color, so it would make sense, and selecting those item makes it return Err(Error::Skipped). (So we don't break API compatibility).

It's a bit clunky but brings some specific API support and perhaps unique visual representation.

Responding to this: Look at the draft I'm making, I changed the way to make it optional just returning a Select<T, Optional> and implementing a SkippablePrompt that it's the same API as PromptInteraction

romancitodev commented 1 month ago

I personally (as a user) think that the suggestion of the first item being "(none)" or similar is the better UX over fumbling with key combinations... not sure I've ever seen a skippable prompt with a key combination to skip tbh...

@cylewitruk Well, crates as promptuity let you handle skippable prompts like selects, indeed I created a lib using promptuity instead of this one because some decisions but now I think it's the time to migrate to this crate.

example of a skippable select prompt

NOTE: I'm not publiciting my CLI or anything, is just a real example.

fadeevab commented 1 month ago

@romancitodev I see. My thoughts:

  1. Code-wise, handling of the Option still leads to a custom match/if-else after the interaction. It also returns different types: Select<T, Optional> and Select<T, Required>. It means, that keeping "visually the same" API isn't necessary: _an alternative could be as simple as introducing an interact_optional method._
  2. Visuals are still not decided. I believe the hint should be rendered automatically.
  3. We've got code duplication in the internals.

The last but not the least, once we introduce the optional select, every prompt will "wish" to be turned into an optional prompt.

Thus, a more universal solution is needed.

@cylewitruk @romancitodev More thoughts:

  1. Around the TUI/visuals with optional interaction: I believe for a selection list item 0 "none" should be a special None case. For an input prompt, the empty line could be a None case. You see where it goes.
  2. API-wise: what's about interact_optional()?
fadeevab commented 1 month ago

@romancitodev Interesting, promptuity also uses @clack/prompt TUI theme... As an observation, cliclack had already been published when the promptuity was started.

cylewitruk commented 1 month ago

Maybe, as an option, the following might be a way to go: skippable (or with_skip_text, whatever) introduces the item 0 with a text of another color, so it would make sense, and selecting those item makes it return Err(Error::Skipped). (So we don't break API compatibility).

It's a bit clunky but brings some specific API support and perhaps unique visual representation.

Tbh i think i'd be fine with something like this as something that provides a solution for the immediate request...

It is a little weird in that skip is an error (which it likely isn't given it was specifically requested).

fadeevab commented 1 month ago

@cylewitruk Just, I realized that it leads to the problem with ? notation: you'd like to handle an actual error event with ease, ?, without implicit knowledge that 1 special error is not an error but some "None" value.

cylewitruk commented 1 month ago

@cylewitruk Just, I realized that it leads to the problem with ? notation: you'd like to handle an actual error event with ease, ?, without implicit knowledge that 1 special error is not an error but some "None" value.

@fadeevab ah yeah that's true, that would be weird.. if you're open to the concept in #72 i think this could be made implicit using that code, i.e. implicitly enable the "skip" function if the result is an Option<T>... then exactly how that's rendered is a separate Q.

fadeevab commented 1 month ago

@romancitodev Meanwhile, I found a genius solution for selection lists that returns an Option<&str> and works out of the box.

let scope = cliclack::select(format!("Select a scope"))
        .initial_value(None)
        .item(None, "(none)", "No project type")
        .item(Some("core"), "core", "core of the repo")
        .item(Some("tools"), "tools", "")
        .item(Some("cli"), "cli", "oh no")
        .interact()?;

@cylewitruk 👆

The 1st argument - value (!) The 2nd argument - displayed name. The 3nd argument - a dimmed hint.

The snippet above should be added to examples and it closes the case.


Overall, I found that "optional" may mean different things for different prompts:

  1. input: empty sting may be considered as None, however, someone needs a specific input string to be considered as None.
  2. select: the 1st element may be considered as None, however, someone needs to define a custom item to be None.
  3. multiselect: when all elements are not selected, or maybe the 1st element is selected and considered as None.
  4. confirm: doesn't support "none", cause it is literally "yes" or "no".
fadeevab commented 1 month ago

I committed examples of optional input and select: https://github.com/fadeevab/cliclack/commit/bfb67d9e8d86e55b75f5248254db6445c93b2572

@romancitodev I really appreciate your efforts despite the fact we disagree on the approaches.

romancitodev commented 1 month ago

well, these examples are really clean!

@fadeevab I really appreciate you too for having an enjoyable discussion with solid arguments!