Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Better handling of user interrupts (fixes #299) #349

Merged
merged 1 commit into from
Nov 7, 2023
Merged

Better handling of user interrupts (fixes #299) #349

merged 1 commit into from
Nov 7, 2023

Conversation

sol
Copy link
Contributor

@sol sol commented Nov 3, 2023

No description provided.

sol
sol commented Nov 3, 2023
View reviewed changes
cabal.project Show resolved Hide resolved
sol
sol commented Nov 3, 2023
View reviewed changes
src/Garn.hs Outdated Show resolved Hide resolved
@sol sol marked this pull request as ready for review November 3, 2023 16:05
@sol sol marked this pull request as draft November 3, 2023 16:11
@sol sol marked this pull request as ready for review November 3, 2023 17:13
@sol sol changed the title Better handling of user interrupts (#299) Better handling of user interrupts (fixes #299) Nov 3, 2023
soenkehahn
soenkehahn approved these changes Nov 6, 2023
View reviewed changes
Copy link
Contributor

@soenkehahn soenkehahn left a comment

Choose a reason for hiding this comment

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

Nice, thanks for the PR! Left some comments.

src/Garn.hs Outdated Show resolved Hide resolved
.gitattributes Show resolved Hide resolved
cabal.project Show resolved Hide resolved
@sol
Copy link
Contributor Author

sol commented Nov 7, 2023

@soenkehahn thanks for the review. I am away from the computer right now, I will do a rebase later.

@sol
Copy link
Contributor Author

sol commented Nov 7, 2023

I will do a rebase later.

Done.

@soenkehahn soenkehahn mentioned this pull request Nov 7, 2023
Merged
@soenkehahn
Copy link
Contributor

@sol: I think the PR makes sense, so I'll merge it. I'm a bit confused by the title: before ctrl+c was also delegated to the child process. Did you experience a case where it wasn't?

@soenkehahn soenkehahn merged commit b021e00 into garnix-io:main Nov 7, 2023
28 checks passed
@sol sol deleted the user-interupts branch November 7, 2023 22:57
@sol
Copy link
Contributor Author

sol commented Nov 7, 2023

I'm a bit confused by the title: before ctrl+c was also delegated to the child process. Did you experience a case where it wasn't?

@soenkehahn see #299.

Also note that before this PR, any process that would call garn run could not detect when the subprocess was canceled due to ctrl-c, which can be a problem in some situations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soenkehahn soenkehahn soenkehahn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sol @soenkehahn