-
Notifications
You must be signed in to change notification settings - Fork 554
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
Fixes #493 Capture exit exception for reference #639
Fixes #493 Capture exit exception for reference #639
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only minor things, looks good to me - thank you very much for your contribution! 👍
but yeah, in general at_exit
is due for a refactoring :)
spec/simplecov_spec.rb
Outdated
end | ||
|
||
context "when an exception has not occurred" do | ||
it "does set the exit_exception" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
has no exit_exception
might be better here :)
spec/simplecov_spec.rb
Outdated
end | ||
|
||
it "returns the exit exception" do | ||
expect(SimpleCov.exit_exception).to be(error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo we already tested this basically in the tests before this, so I'd be good without this spec :)
Thanks for the feedback. I'll implement the fixes in the next couple days 👍 |
This stores any exit exception that may have occurred. The idea is to expose this exception to the user supplied ```at_exit``` block. ```ruby SimpleCov.at_exit do if SimpleCov.exit_exception # Do something or nothing due to an exception else # Do something or nothing due to no exception end end ``` Another method was added: ```exit_status_from_exception``` to reduce the logic in the at_exit block. I am not sure if it's worth adding this method though. There is still opportunity to refactor the ```at_exit``` block. I first attempted to refactor the @exit_status logic more but it will be a bigger change. If you want I can remove the ```exit_status_from_exception``` method from this PR
eb7e5b8
to
ecee188
Compare
and by next couple of days... i apparently meant the next 10 days.... :) OK, ready for another review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries, this is OSS - everyone in their own time!
Thanks a bunch 👍
Fixes #493
This stores any exit exception that may have occurred. The idea is to
expose this exception to the user supplied
at_exit
block.Another method was added:
exit_status_from_exception
to reduce thelogic in the at_exit block. I am not sure if it's worth adding this
method though. There is still opportunity to refactor the
at_exit
block. I first attempted to refactor the @exit_status logic more but
it will be a bigger change. If you want I can remove the
exit_status_from_exception
method from this PRI also don't love having
begin rescue end
blocks in tests, but I wasn't sureif there was a better way to test that. Maybe I could wrap
$!/$ERROR_INFO
in another method and mock that in testing.