mokkabonna / inquirer-autocomplete-prompt

Autocomplete prompt for inquirer
ISC License
350 stars 82 forks source link

`alwaysValidate` override? #59

Closed StevenLangbroek closed 3 years ago

StevenLangbroek commented 6 years ago

I have a use-case for an attribute that I'd like to contribute, but before I do I'm curious if it's something you'd be open to accepting:

New property: alwaysValidate: boolean (default: false)

Generically speaking, there's something to be said for forcing validation, even if the list is limited. To keep inquirer's pizza example, what if the Pizza API has an ingredients API, but a separate one to confirm whether they have an ingredient in supply?

In my use-case, I want to scaffold a test for a component. I do a glob for component files in the project to prepopulate the list, but then have to do some poking around to see if there's a test already maybe (we have some non-standard locations so I have to brute-force it a bit).

I can write the code & tests myself and submit a PR, but I thought it prudent to gauge interest first :).

karlhorky commented 6 years ago

Hey Steven, thanks for the feature request!

I thought about it a bit, and maybe it's possible that this would work as a default? So, if you have a validate function, that it will be used regardless of the suggestOnly: true option? What do you think of this?

If this sounds ok to you, would you be open to exploring it a bit? For example, demonstrating both how it works if the suggestOnly option is set to false as well as true? And testing out any possible failure cases for it?

The original commit that introduced the check was 7a31d78fdaf9bfe7c6d9bd712bad01eb053574c8 (from https://github.com/mokkabonna/inquirer-autocomplete-prompt/pull/21). I'm not sure if @mokkabonna had a particular reason that it would fail if suggestOnly was set to false. Maybe there's something that needs to be taken into account, so that we don't break the prompt.

StevenLangbroek commented 6 years ago

Yep definitely. The original behaviour was a bit unexpected but it was documented and reasonable, so I prefer what you describe. It'd be a breaking change though I think officially, so Major +1?

I'm not sure if @mokkabonna had a particular reason that it would fail if suggestOnly was set to false.

I guess I could see that if the options are already limited to a predefined list, you could argue that it then shouldn't need more validation, you should've narrowed your list down further. In my case though, I don't want to run this validation on every file beforehand, I just want it to run on an individual entry after selection (we read a file, parse its AST and see if it qualifies for a certain transform).

karlhorky commented 6 years ago

Yep, at least at first glance it feels like a valid use case, allowing use of the existing Inquirer feature in order to enable things like increasing efficiency by doing expensive extended validation only on the single option chosen. Seems like this would also enable other workflows too.

This does change the behavior which downstream projects may be relying upon, so I suppose a major bump would make sense. #64 will land in the next few days probably, so maybe base work on that.

Want to take a look at this then?

If this sounds ok to you, would you be open to exploring it a bit? For example, demonstrating both how it works if the suggestOnly option is set to false as well as true? And testing out any possible failure cases for it?

StevenLangbroek commented 6 years ago

Sorry missed notification for this. I'll work on this on Tuesday, should have it in the same day. It's not that much work afaict. Thanks for the input ✌️

karlhorky commented 6 years ago

Cool, no problem, #64 has been merged, so make sure to rebase your changes :)

moshfeu commented 3 years ago

It's been a while :) Any plan to implement it?

mokkabonna commented 3 years ago

Yes I have decided to implement this and have started with the task. But not with an option like here. I think that is not necessary. But rather validate whenever a validation function is present.

mokkabonna commented 3 years ago

Any comments? https://github.com/mokkabonna/inquirer-autocomplete-prompt/pull/120

mokkabonna commented 3 years ago

Merged and released as v1.3.0