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.

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

error handling when checking for stack binary #1219

Merged
merged 2 commits into from Apr 20, 2016

Conversation

Projects
None yet
2 participants
@bermanjosh
Copy link
Contributor

commented Apr 20, 2016

For #1218

@@ -79,8 +79,8 @@ keter cabal noBuild noCopyTo buildArgs = do

unless noBuild $ do
stackQueryRunSuccess <- do
(ec,_,_) <- readProcessWithExitCode "stack" ["query"] ""
return (ec == ExitSuccess)
eres <- try $ readProcessWithExitCode "stack" ["query"] "" :: IO (Either SomeException (ExitCode, String, String))

This comment has been minimized.

Copy link
@snoyberg

snoyberg Apr 20, 2016

Member

Catching all exceptions (via SomeException) is dangerous, as it can swallow up async exceptions. Does catching just IOException fix this?

@@ -1,5 +1,5 @@
name: yesod-bin
version: 1.4.18
version: 1.4.19

This comment has been minimized.

Copy link
@snoyberg

snoyberg Apr 20, 2016

Member

It would be better to make a patch version bump (1.4.18.1)

@bermanjosh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2016

@snoyberg You're right, I'll double-check the exception type and make those changes tonight / tomorrow morning.

@snoyberg

This comment has been minimized.

Copy link
Member

commented Apr 20, 2016

Awesome, thanks!

On Wed, Apr 20, 2016 at 7:28 PM Josh Berman notifications@github.com
wrote:

@snoyberg https://github.com/snoyberg You're right, I'll double-check
the exception type and make those changes tonight / tomorrow morning.


You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#1219 (comment)

@bermanjosh

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2016

@snoyberg Done

@snoyberg snoyberg merged commit 0761cd7 into yesodweb:master Apr 20, 2016

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@bermanjosh bermanjosh deleted the bermanjosh:yesod-keter-no-stack branch Apr 21, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.