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

Add OnInterrupt to run function code on CTRL+C #301

Merged
merged 7 commits into from
Dec 11, 2020
Merged

Add OnInterrupt to run function code on CTRL+C #301

merged 7 commits into from
Dec 11, 2020

Conversation

infalmo
Copy link
Contributor

@infalmo infalmo commented Oct 1, 2020

No description provided.

@infalmo
Copy link
Contributor Author

infalmo commented Oct 4, 2020

@AlecAivazis I'm interested in managing this repository.
Would it be fine if you added me as a collaborator?

@AlecAivazis
Copy link
Owner

Hey @infixint943!

Thanks for submitting this. Do you mind adding tests and some documentation to go with the new feature?

Also, I appreciate the offer to help maintain this library. For now, the best way to help is to answer questions that pop up on issues and keep submitting PRs.

@AlecAivazis
Copy link
Owner

AlecAivazis commented Oct 5, 2020

Thinking about this API more i think its important that we don't just add something to the top-level API. we probably want to have a survey.Option which lets us provide a handler for specific calls to Ask/AskOne. We could still provide a global default handler if you think that's important

@infalmo
Copy link
Contributor Author

infalmo commented Oct 5, 2020

Thinking about this API more i think its important that we don't just add something to the top-level API. we probably want to have a survey.Option which lets us provide a handler for specific calls to Ask/AskOne. We could still provide a global default handler if you think that's important

Usually, I end up terminating the process when I encounter a CTRL+C, so I found a global configuration more viable (rather than configuring the function every time I do Ask/AskOne).
However, a better alternative would be to add an option in the survey.Option that allows for overriding the global configured function.

How good would this inclusion be?

@AlecAivazis
Copy link
Owner

I think having both is the right move 👍

@AlecAivazis
Copy link
Owner

@infixint943 any progress on adding the option?

@infalmo
Copy link
Contributor Author

infalmo commented Oct 15, 2020

Hey!
I was caught up in some school work, and I forgot about it. I'll work on it tonight.
Thanks for the reminder!

@infalmo
Copy link
Contributor Author

infalmo commented Oct 15, 2020

Should I rename the global OnSIGINTFunc to something different?

@AlecAivazis
Copy link
Owner

I think something simply like OnSigint is probably good enough

@infalmo
Copy link
Contributor Author

infalmo commented Oct 16, 2020

Done!
If the current method is alright, shall I proceed with writing tests?

@AlecAivazis
Copy link
Owner

Thinking about this name some more, i think OnInterrupt is probably more clear. and to answer your question, yea it looks good!

@infalmo
Copy link
Contributor Author

infalmo commented Oct 21, 2020

@AlecAivazis Can you please investigate why tests keep timing out randomly, in my PR?

@AlecAivazis
Copy link
Owner

Hey @infinitepr0, sorry you are running into timeout issues with the tests. Unforunately, i dont have a lot of free time to hunt this down right now :(

@infalmo
Copy link
Contributor Author

infalmo commented Nov 4, 2020

This may sound stupid, but how do I make the tests send a SIGINT to the prompt?
Here's what I've done so far

func TestOnInterruptFunc(t *testing.T) {
	// Test global function.
	OnInterrupt = func() {
		fmt.Println("The end!")
	}

	RunPromptTest(t, PromptTest{
		name: "Test global OnInterrupt Func.",
		prompt: &Input{
			Message: "What is your name?",
		},
		procedure: func(e *expect.Console) {
			e.ExpectString("What is your name?")
			// How do I send a SIGINT next?
		},
	})
}

@AlecAivazis
Copy link
Owner

@infinitepr0 any progress?

@infalmo
Copy link
Contributor Author

infalmo commented Nov 18, 2020

Sorry for the delay (and thanks for the reminder).

@infalmo
Copy link
Contributor Author

infalmo commented Nov 18, 2020

Requesting you to review this PR @AlecAivazis

@infalmo infalmo changed the title Add OnSIGINTFunc to run function code on CTRL+C Add OnInterrupt to run function code on CTRL+C Nov 19, 2020
Copy link
Owner

@AlecAivazis AlecAivazis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind explaining why you changed the way errors are handled in tests?

@@ -28,7 +28,6 @@ func RunTest(t *testing.T, procedure func(*expect.Console), test func(terminal.S
}()

err = test(Stdio(c))
require.Nil(t, err)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was this intentionally removed? If so, can you explain why?

Copy link
Contributor Author

@infalmo infalmo Nov 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, because, L#30 would return an error when SIGINT was encountered, and the tests would fail (due to the assert). So I shifted the error assertion to the helper functions (RunPromptTest and the other one).

I could revert this, but that would mean rewriting another RunTest function exclusively for the OnInterrupt test, which isn't a very good idea imo.

@infalmo infalmo requested a review from AlecAivazis November 27, 2020 21:39
@infalmo
Copy link
Contributor Author

infalmo commented Dec 10, 2020

@AlecAivazis Sorry for disturbing, but can you please review the PR and merge it is all's well.

Thanks.

@AlecAivazis
Copy link
Owner

Sure thing! Thanks for the reminder 👍

@AlecAivazis AlecAivazis merged commit 90b418e into AlecAivazis:master Dec 11, 2020
@infalmo
Copy link
Contributor Author

infalmo commented Dec 11, 2020

Thanks a lot!

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.

2 participants