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

Safely optional #73

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@kennethkalmer
Owner

kennethkalmer commented Sep 12, 2013

Based on some great feedback from @YorickPeterse, I'm making safely usage completely optional, albeit turned on by default.

The touchpoints were minor, so this should just work. Will play a bit a see before merging.

Feedback welcome.

/cc @marcbowes

@marcbowes

This comment has been minimized.

Show comment
Hide comment
@marcbowes

marcbowes Sep 13, 2013

Collaborator

Tests? :-)

I think safely doesn't have to be a dev dependency.

Collaborator

marcbowes commented Sep 13, 2013

Tests? :-)

I think safely doesn't have to be a dev dependency.

@kennethkalmer

This comment has been minimized.

Show comment
Hide comment
@kennethkalmer

kennethkalmer Sep 13, 2013

Owner

I made it a development dependency so I could flesh out the tests :)

Thanks for repremanding me 👍

Making commit comments where the tests should be

Owner

kennethkalmer commented Sep 13, 2013

I made it a development dependency so I could flesh out the tests :)

Thanks for repremanding me 👍

Making commit comments where the tests should be

@@ -105,7 +105,8 @@ def run_amqp!
cmdq = mq.queue( q, :durable => true )
cmdq.subscribe( :ack => true ) do |header, message|
safely do
receiver = lambda do |header, message|

This comment has been minimized.

@kennethkalmer

kennethkalmer Sep 13, 2013

Owner

Definitely needs specs as per @marcbowes instructions

@kennethkalmer

kennethkalmer Sep 13, 2013

Owner

Definitely needs specs as per @marcbowes instructions

@marcbowes

This comment has been minimized.

Show comment
Hide comment
@marcbowes

marcbowes Sep 13, 2013

Collaborator

At the moment, Thread.abort_on_exception=true regardless of whether you have Safely or not. I think I agree with this, however this is done in the configure_safely method. I think you should rename this method to something else, e.g. configure_exception_handling, and then extract the safely stuff to configure_safely. You can then invoke that method if Safely is defined, ala configure_safely if safely_enabled?. The safely_enabled? method can be stubbed under test to return false for the case where you want to assert that it won't be configured when the gem isn't available. This is better than fiddling with constants at test time.

Collaborator

marcbowes commented Sep 13, 2013

At the moment, Thread.abort_on_exception=true regardless of whether you have Safely or not. I think I agree with this, however this is done in the configure_safely method. I think you should rename this method to something else, e.g. configure_exception_handling, and then extract the safely stuff to configure_safely. You can then invoke that method if Safely is defined, ala configure_safely if safely_enabled?. The safely_enabled? method can be stubbed under test to return false for the case where you want to assert that it won't be configured when the gem isn't available. This is better than fiddling with constants at test time.

@kennethkalmer

This comment has been minimized.

Show comment
Hide comment
@kennethkalmer

kennethkalmer Sep 13, 2013

Owner

💯 times { 👍 }

Owner

kennethkalmer commented Sep 13, 2013

💯 times { 👍 }

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