Skip to content
This repository has been archived by the owner on Apr 19, 2024. It is now read-only.

add Confirm prompt #30

Merged
merged 3 commits into from
Apr 4, 2017
Merged

add Confirm prompt #30

merged 3 commits into from
Apr 4, 2017

Conversation

coryb
Copy link
Collaborator

@coryb coryb commented Apr 3, 2017

Adding basic Y/n confirmation prompt. The implementation is similar to MultiChoice in that I have added an Answer that users can populate to get a bool result, otherwise it returns a Yes or No string depending on the answer. This will also loop if we get an answer that we cannot parse.

$ go run ./examples/confirm.go
? Are you happy? (Y/n) Yeah
✘ Sorry, your reply was invalid: "Yeah" is not a valid answer, try again
? Are you happy? Yes
response string: Yes
response happy: true

@AlecAivazis
Copy link
Owner

Hey Cory, thanks for yet another PR. I'll leave some comments inline.

I did some thinking about our problem today and what do you think about extending the analogy with json.Unmarhsal and changing the API of Ask to receive a pointer to a struct where the values for the entire survey is stored. We could use field tags to link up a question name with the field if we didn't want to enforce some conventions. This allows us to avoid the whole type problem to begin with from the consumer's perspective. It doesn't really affect this PR (since it makes 100% sense for Confirm to return a boolean), but I wanted to get your thoughts.

Copy link
Owner

@AlecAivazis AlecAivazis left a comment

Choose a reason for hiding this comment

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

Pretty good overall! I think we should be a bit more defensive. Since the Answer field is not verified anywhere, it's easy to get a null-pointer reference if you fmt.Println(prompt.Answer)

}

// return the value
*confirm.Answer = answer
Copy link
Owner

Choose a reason for hiding this comment

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

What if the user doesn't provide an answer when instantiating this prompt?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

doh! I meant to add that logic, forgot. Like in MultiChoice I just check to see if the value is nil and then create a default answer.

@AlecAivazis
Copy link
Owner

Awesome! Thanks for making those changes. I'm going to merge this in now.

@AlecAivazis AlecAivazis merged commit 3d4e271 into AlecAivazis:master Apr 4, 2017
siredmar pushed a commit to siredmar/survey that referenced this pull request Feb 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants