add Checkbox prompt

[coryb]

I have refactored some of the common code from Choice and Checkbox into multioptions.go. The event loop it mostly the same, but didnt see an obvious way to abstract it away since I need to handle the "space" key differently for Checkbox.

One issue I not sure how to work around is that a Checkbox should return the slice of options checked, but we are limited by the Prompt() (string, error) interface which only allows a single string to be returned. My work-around is to just return the list joined by , character. Users will need to split the answer to get back to a useful result. Not sure if you have ideas there.

[AlecAivazis]

Hmm you bring up a good point about the interface preventing multiple values being returned from a single prompt... I'll have to think about the right way to do that. Thanks for submitting this, I'll take a look later tonight and let you know if I have any feedback or come up with something - what do we do if the user's response contains a ,?

[coryb]

btw, I noticed you reverted the ➤ to >. I assume this was to better handle terminals that don't support unicode? The checkbox patch uses ❯, ◉ and ◯ for options. I can change those to >, X and O respectively if desired.

Also the validation error changed from ! to ✘ in my previous patch, I can revert that one also if you prefer.

I was just trying to mimic what I saw from similar inquirer.js output, but honestly I am not sure how it handles ascii-only terminals.

Thanks!
-Cory

[AlecAivazis]

Yea, i tagged you in the PR reverting the change. I showed the change to a few people around me and they preferred > to ➤ because ➤ looked too bold compared to the rest of my terminal's text. I'm fine with unicode in general. Feel free to keep all the other unicode characters as they are.

Somewhat related, I have been thinking of adding a flag that would simplify the interaction down to what npm has for older terminals, flipping to these more primitive ones woulud be a great thing to add to that mode.

[coryb]

Okay, cool, I just saw the other PR, thanks! I will think about ways to tweak the symbols to allow users to downgrade to ascii if desired without having to copy/paste the entire template.

[AlecAivazis]

Hey Cory, so I did some thinking on this and i'm curious to hear your thoughts. I think the , separated value is a great solution since the interface requires returning a string. It does have the unfortunate issue if the user's response contained a , but that's easily solved with using something like JSON instead of just , to encode the multiple values using a single string.

I wonder if this issue is because the interface for Prompt is too limiting. If instead of returning a single string, we allowed Prompt to return interface{} then we could handle arbitrary outputs, and not have to deal with this problem again. This has the unfortunate side effect of requiring a cast to a known value in order to get any values of out the response, but we might be able to hide that behind some more types. For example, rather than returning a map[string]string, survey.Ask could return a Response type that has getString, getInt, getBool, etc. methods which perform the type casting, saving the user from the somewhat ugly code and apprent loss of type safety. Although, i'm not sure how this would work with listOf. Once I started down that hole, it felt like just returning an interface{} and then forcing the user to cast, was the right comprimise.

thoughts?

[coryb]

Hey Alec, a few thoughts,

first on the symbol replacement, in retrospect I dont think it is too much of a burden to have users fix up the templates to use the symbols desired. For instance if they want to use ascii for Checkbox then this code (although a bit ugly) would get the job done:

    CheckboxOptionsTemplate = strings.Replace(
        strings.Replace(
            strings.Replace(CheckboxOptionsTemplate, "◯", "O", -1),
            "◉", "X", -1),
        "❯", ">", -1)

Or with a bit more clarity:

    replacements := map[rune]rune{
        '◯': 'O',
        '◉': 'X',
        '❯': '>',
    }
    translateRunes := func(r rune) rune {
        if t, ok := replacements[r]; ok {
            return t
        }
        return r
    }
    CheckboxOptionsTemplate = strings.Map(translateRunes, CheckboxOptionsTemplate)

Now, for the Prompt interface change ... a type cast would work but feels pretty kludgy to me. I am wondering if it makes more sense to follow the model used by json.Unmarshal where the answer is passed in as a data pointer to be populated by the survey routines. So specifically thinking something like this:

type Validator func(answer interface{}) error
type Prompt interface {
    Prompt(answer interface{}) error
    Cleanup(answer interface{}) error
}

The main routines in survey would looks something like:

func AskOne(p Prompt, ans interface{}) error
func AskOneValidate(p Prompt, ans interface{}, v Validator) error
func Ask(qs []*Question) error

Then modify the Question struct to look like:

type Question struct {
    Name     string
    Prompt   Prompt
    Validate Validator
    Answer   interface{}
}

Where a concrete example of would look like:

    var days = []string{}
    var simpleQs = []*survey.Question{{
        Name: "days",
        Prompt: &survey.Checkbox{
            Message:  "What days do you prefer:",
            Options:  []string{"Sunday", "Monday", "Tuesday", "Wednesday", "Thursday", "Friday", "Saturday"},
            Defaults: []string{"Saturday", "Sunday"},
        },
        Answer: &days,
    }}
    err := survey.Ask(simpleQs)
    if err != nil {
        panic(err)
    }

    fmt.Printf("prefers: %v.\n", days)

I tweaked the code to get a proof of concept and it seems to work. The mess will be with us trying to assign to the passed in interface, so that will either look like a switch/type statement or worst case using reflection. At least in this case the client code will be type safe and we would just have to ensure type safety or return errors.

-Cory

[coryb]

Thinking a bit further, there might be a way to just embed the answer inside the prompt structs like:

        Prompt: &survey.Checkbox{
            Message:  "What days do you prefer:",
            Options:  []string{"Sunday", "Monday", "Tuesday", "Wednesday", "Thursday", "Friday", "Saturday"},
            Defaults: []string{"Saturday", "Sunday"},
            Answer: &days,
        },

Then that would let the interfaces look like:

type Prompt interface {
    Prompt() error
    Cleanup() error
}

This would allow concrete types throughout with out any switch/type statements or reflection since inside the logic for each prompt we have guarantees of concrete types. ie Checkbox would look like:

type Checkbox struct {
    Message  string
    Options  []string
    Defaults []string
    Answer   *[]string
}

We just have to verify that p.Answer != nil inside the Prompt functions.

I will sleep on it a bit and see if this all sounds crazy tomorrow :)
-Cory

[coryb]

Hey Alec, other than the multi-value discussion, were there any other changes you want with this PR? I was thinking perhaps the best course of action would be to merge this PR (barring any changes you want) and then we could retrofit this in a secondary PR to deal with any API changes that would be required.

You might also want to add a "v0" or a "v1" tag to the repo so that users could always import with something likegopkg.in/AlecAivazis/survey.v0 to get to an API compatible version (assuming you want to make changes that will break backwards compatibility).

-Cory

[AlecAivazis]

Hey Cory, thanks for the excellent write up. I'm going to have to think really hard about what to do about this. Also sorry I got a bit distracted. I'm coming to the final stretch of another project i've been working on for a while and that has been keeping my attention away from this problem.

I think I prefer to do a mix of what you suggest at the end of your comment. I'd like to keep the interface the same (so checkbox should still return something) but i think I prefer a json encoded list of strings instead of joining them with , (that way we don't lose information). It's a bit annoying/slow to decode but that's where the other half comes in. I think it's also a good idea to provide the place to store the answers as an optional field to Checkbox which would be written to before Prompt returns the jsonified string. It's definitely far from ideal, but I think this way, i'm not totally annoyed when using Checkbox (I need it badly haha) and it avoids any major changes to the library.

In the meantime, I'm going to do exactly what you suggest at gopkg which will allow me to break the API with confidence 😄 .

How does that sound?

[AlecAivazis]

Also, another very minor point but while I've been taking a lot of inspiration from inquirer, I find their names a bit misleading - a checkbox of one option is also a confirm. I prefer naming this prompt something more along the lines of MultiSelect which indicates that it can provide multiple answers. How does that sound? I'm definitely open to other ideas if you have them.

[coryb]

Okay, sounds good. I will modify the PR to return JSON for Checkbox and also add an Answer field to the Prompt structs. I think it is a good compromise.

I will also rename Checkbox to MultiSelect (I don't actually use inquirer directly so I have no opinions on the naming). Perhaps an alternative would be MultiChoice since you already have the Choice prompt.

Thanks for letting me harass you over the weekend :)

[AlecAivazis]

Not problem at all! Thanks for taking the time to help me build up survey!

Just to make sure I conveyed what I was thinking, I don't think we need to add the answers field to any of the other prompts except the MultiSelect since that's the only one that can return non-string values.

I'll spend some time thinking about the name on my way to work and if Inhavent formed any opinions by then we'll just go with 'MultiChoice' like u suggest

[coryb]

Just curious why you don't want the Answer fields in all of the Prompt structs? It is redundant for the simple string types but I think the consistency of having them all have Answer fields is probably a "good thing" ™️.

[AlecAivazis]

I want to be really careful before we make any changes to the interface since we might decide to do something completely different and breaking the API can cause be annoying for consumers. By having Answer only on the MultiSelect / MultiChoice we can get the feature without larger implications to the library's API. Seems like the right balance until we have a better sense of wether this is the right pattern to follow everywhere. If we do decide to make it unanimous, there is some work to do in removing the redundancy so i'd prefer to avoid having two different paths for collecting answers except in the one place it sort of has to be for sanity.

[AlecAivazis]

Thanks for making those changes @coryb I'm going to merge this now. Please let me know if you have any more ideas for this library, i appreciate all your feedback.