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 'interact_text_opt' and 'interact_text_on_opt' for 'Input' prompts #278

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

tbergerd
Copy link
Contributor

@tbergerd tbergerd commented Sep 10, 2023

Hello,
This adds 'interact_text_opt' and 'interact_text_on_opt' for 'Input' prompts (see #160)

Note I did not do the same for 'interact' methods, I don't see how this would work with escape sequences being accepted.
Additionally, unlike selection prompts, I only allowed cancellation via the 'Esc' key, considering 'q' is a valid input

P.S: Is there a specific process to contribute or is opening a pull request fine ?

@tbergerd tbergerd changed the title Input opt Add 'interact_text_opt' and 'interact_text_on_opt' for 'Input' prompts Sep 10, 2023
src/prompts/input.rs Outdated Show resolved Hide resolved
@@ -339,11 +338,6 @@ where
},
)?;

// Read input by keystroke so that we can suppress ascii control characters
if !term.features().is_attended() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note : removed this because the check is already performed at the beginning of the function (and returns an error instead which makes more sense)

Copy link
Contributor

@Gordon01 Gordon01 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, Please restructure the code a little and it's good to go.

self._interact_text_on(term, true)
}

fn _interact_text_on(mut self, term: &Term, allow_quit: bool) -> Result<Option<T>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

You should follow a builder pattern here, so the signature of this function shouldn't change.
Instead, add a new private field to the Input, and a corresponding setter for it.
Access it here via self.

render.clear()?;
term.flush()?;
term.show_cursor()?;
return Ok(None);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's instead add a new variant for the Error like UserInterrupted and return it here.

@@ -281,20 +280,47 @@ where
impl<T> Input<'_, T>
where
T: Clone + ToString + FromStr,
<T as FromStr>::Err: Debug + ToString,
<T as FromStr>::Err: ToString,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you removing this bound?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the bound was only required by the call to unwrap (in the dead code I removed in the same commit : 05f6a3b)
I figured I might as well remove it since it seems unnecessary (interact and interact_on didn't have this constraint).

Copy link
Contributor

Choose a reason for hiding this comment

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

Then it's definitely worth fixing. I suggest making a separate PR for this and removal of the redundant check.

@Gordon01
Copy link
Contributor

Gordon01 commented Sep 11, 2023

Oh, I see why you took this approach. Other prompt implementations are like this.
Personally, I think that the builder pattern is cleaner. Single-level Result is easier to handle in the user's code, Error variants are easily matchable and directly compatible with crates like anyhow.

@pksunkara what do you think? Should "cancel" implementations be identical in all prompts?

@pksunkara
Copy link
Collaborator

@pksunkara what do you think? Should "cancel" implementations be identical in all prompts?

Yes, but IIRC there were some complications on the Input prompt, and I need to review this PR closely this weekend.

@tbergerd
Copy link
Contributor Author

Thank you for the review.

Yes I basically copied what was done for other prompts.
If not for consistency, I agree with your points : builder methods + an extra error variant and a single 'interact' would be better (for the impl complexity, the API surface and the calling code)

One thing to note is this wouldn't work with interact and interact_on (as is currently the case with 'completion_with' and 'history_with') though. I'm not quite sure what the use case for those methods is actually (unicode input probably ?)

If it were up to me I'd say consistency should prevail, I'll wait for both your feedbacks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants