jdx / demand

prompt library for rust
https://docs.rs/demand
MIT License
52 stars 3 forks source link

scoped threads and less restrictive trait bounds #47

Closed Vulpesx closed 4 months ago

Vulpesx commented 4 months ago

I found Spinner.run and DemandOption to be overly restrictive in functionality that made it rather annoying to use

Restrictive trait bounds

firstly, DemandOption requiring item to be display is rather ridiculous, after all it's the label that gets displayed even if the label is taken from the item.

So the item doesn't require display now, the new fn does the same thing it did before (making the label from the item) but it requires ToString instead because while before it required Display you were actually using ToString which you could do since ToString is impl on all impl Display. Requiring ToString is more accurate and allows types that impl ToString but not Display

Since new still requires item to be ToString we need another fn that allows us to take advantage of item not needing Display anymore. That's what DemandOption::with_label does, it takes a separate label and item, the label being Into<String> and item being just T. There is also .item fn to be consistent with the rest of the lib.

Select and MultiSelect do not require Display either. For some reason, Select was using item.to_string instead of the label to render, which is the only place ToString and by extension Display was ever needed for item.

Scoped threads

the old Spinner.run required a static Fn which means the Fn can only move or take static references which is really annoying especially since It's clearly not necessary since run will not return unless the thread is done. Thankfully, rust has scoped threads which remove this limitation. As a bonus I changed run to take a FnOnce which is more appropriate as it's only run once, and it allows for more closure magic, as well as have run return the value the FnOnce returns. If the thread running the FnOnce panics run will return an io::Error with the kind being Other (which exists precisely for when you need to return any error as an io::Error) with the value being the panic message.

Just to be clear. None of the changes in this pr affect any code already using demand. You can just do more now

Vulpesx commented 4 months ago

I had an idea while writing this, that you should be able to update the spinner while it's running (probably through a mspc) so you could show new info in the spinner as your code progresses. I'll make another pr for that, unless you want it in this one. Also, if you want me to split the changes in this into different pr's I can.

jdx commented 4 months ago

wonderful changes! thank you!

roele commented 4 months ago

👍🏼 can only second that, great contribution!