search

AlecAivazis / survey

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

Revert "Add OnInterrupt to run function code on CTRL+C" #323

Closed AlecAivazis closed 3 years ago

AlecAivazis commented 3 years ago

@mislav @infinitepr0 - I'm going to revert #301 and republish on the v2.2.x line.

Also, before we merge it back in - @infinitepr0 is there an issue with just relying on the return value from Ask or AskOne in order to capture an interrupt?

infalmo commented 3 years ago

Sorry for the delay, was caught up with some exams!

Okay, so the actual reason I pushed forward for such a feature is:

  1. Most projects using this project are CLI tools. ALL tools exit instantly on CTRL^C. Handling this seperately for just this library, especially when the input prompter is used tens of times, is really cluttering and unneccessary. Instead, running a os.Exit(1) on encountering a CTRL^C does it.
  2. If coded well, the only possible error returned by the library would be InterruptError. Using the above idea would mean we could potentially omit the error field.

This was my usecase in the project I've built (has atleast 25 prompts) so I found it much better to have this feature.

mislav commented 3 years ago

@infinitepr0 Thanks for explaining! I'm not a maintainer of this project, but if I were, I would have valued seeing such description in the body of your original PR, instead of receiving a PR for a new feature with empty description. When feature requests are argued for with clear intentions, maintainers of the project can engage in better decision-making when it comes to deciding whether something is a good fit for the project.

I understand that adding such a feature cleans up repetitive error handling in your application where you have many prompts using Survey. However, one of the unavoidable things about Go in general that error handling is repetitive. Speaking as someone who maintains a codebase that heavily depends on Survey, what I would have done differently if I were to start over would be hiding all calls to Survey behind a package of our own. That way, you could bake centralized error handling around Survey into your application without having to add a special error-handling feature to Survey.

infalmo commented 3 years ago

@infinitepr0 Thanks for explaining! I'm not a maintainer of this project, but if I were, I would have valued seeing such description in the body of your original PR, instead of receiving a PR for a new feature with empty description. When feature requests are argued for with clear intentions, maintainers of the project can engage in better decision-making when it comes to deciding whether something is a good fit for the project.

I understand that adding such a feature cleans up repetitive error handling in your application where you have many prompts using Survey. However, one of the unavoidable things about Go in general that error handling is repetitive. Speaking as someone who maintains a codebase that heavily depends on Survey, what I would have done differently if I were to start over would be hiding all calls to Survey behind a package of our own. That way, you could bake centralized error handling around Survey into your application without having to add a special error-handling feature to Survey.

Yes, I think your answer is a befitting reply to my PR. I've decided to follow your advice and submodule the survey library to fit my needs. Thanks again, and sorry for the inconvenience caused!