AlecAivazis / survey

A golang library for building interactive and accessible prompts with full support for windows and posix terminals.
MIT License
4.07k stars 350 forks source link

Ensure accurate selected index returned from Select #420

Closed mislav closed 1 year ago

mislav commented 2 years ago

Fixes https://github.com/AlecAivazis/survey/issues/412 which describes the case where the selected index would be mis-reported when some options happen to share identical value. Since the Select component internally tracks the selectedIndex, just use that value instead of trying to determine it by comparing strings.

@AlecAivazis In my attempts to clean up default value handling here, I fear I have introduced backwards-incompatible changes. I could restore the previous behavior, sure, but I wanted to ask if it's also worth "fixing" the default value handling. Namely:

So, what I changed here felt like fixing bugs to me, but I can imagine that some users might be relying on these features, for example having a Default that's not among Options so they can detect when the user wanted to skip a Select. I'm not sure if those features were worth keeping, though. For example, consider this prompt:

? Choose a country:  [Use arrows to move, type to filter]
> Afghanistan
  Åland Islands
  Albania

If I press Enter here, I would expect that "Afghanistan" got selected. However, if there was a Default value such as "moo", that string will be used instead of "Afghanistan", which I find counter-intuitive. I would rather break this feature than have users rely on it.

What are your thoughts?

AlecAivazis commented 2 years ago

Oh, man. This is one of those super tough situations. I agree with you that these are really bug fixes but it seems very likely that someone out there is relying on the silent failure or doesn't know they have an invalid default value.

I'm leaning more towards keeping this under v2 and we should just make sure its very clear what happened in the release notes.

AlecAivazis commented 1 year ago

@mislav i'm going to merge this since it's been around for so long and we both agree it's an appropriate change

mislav commented 1 year ago

@AlecAivazis Thanks! Do you still think this should be a v2? Strictly speaking, I think it should.