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

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

Merged
merged 1 commit into from
Dec 17, 2020

Conversation

AlecAivazis
Copy link
Owner

@AlecAivazis AlecAivazis commented Dec 17, 2020

@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?

@AlecAivazis AlecAivazis merged commit 85b98f3 into master Dec 17, 2020
@infalmo
Copy link
Contributor

infalmo commented Dec 20, 2020

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
Copy link
Collaborator

mislav commented Dec 22, 2020

@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
Copy link
Contributor

infalmo commented Dec 22, 2020

@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!

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.

None yet

3 participants