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

Enhanced Sidekiq and SuckerPunch configuration #73

Merged
merged 9 commits into from
Nov 19, 2013

Conversation

jeremyvdw
Copy link
Contributor

This is a refactor exercice around Rollbar::Configuration#use_sidekiq= and Rollbar::Configuration#use_sucker_punch= methods which has led to a much better isolated classes Rollbar::Delay::Sidekiq and Rollbar::Delay::SuckerPunch.
Those classes instance variables responds to #call which allows to mock them for proc or lambda and behave like an ordinary Rollbar::Configuration#async_handler.

… api

Rollbar::Delay::Sidekiq now respond to #call which allows it to be bound to `Rollbar::Configuration#async_handler`.
Cleans up Rollbar::Configuration api: use sidekiq by calling `#use_sidekiq(options)` instead of `#use_sidekiq=(options)`.
Rollbar::Delay::Sidekiq specs are now isolated.
…ckerPunch to match previous async API changes.

Added SuckerPunch spec
..around `Rollbar::Configuration#use_sidekiq=` and `Rollbar::Configuration#use_sucker_punch=` to match new syntax.
@brianr
Copy link
Member

brianr commented Nov 4, 2013

This looks great on first read. We'll need to provide migration instructions in the release notes, since anyone with

config.use_sucker_punch = {
  ...
}

in config/initializers/rollbar.com will need to update their code after upgrading.

Will take a deeper look soon (possibly today, definitely within the next few days).

@jeremyvdw
Copy link
Contributor Author

@brianr I've added methods for old config directives to work as is, with deprecation warning.

Spec with Ruby 1.8.7 are failing though, because of Sidekiq inclusion.
I'm thinking about using RSpec tag to filter out sidekiq related specs for Ruby 1.8.7, would it make sense to you?

@brianr
Copy link
Member

brianr commented Nov 5, 2013

@jeremyvdw very nice.

Yes, I think that makes sense. Sidekiq isn't supported on ruby 1.8 anyway.

@sbezboro
Copy link
Contributor

One thing that would be cool is if there were tests for the deprecated setter methods. Otherwise looks good to me!

sbezboro added a commit that referenced this pull request Nov 19, 2013
Enhanced Sidekiq and SuckerPunch configuration
@sbezboro sbezboro merged commit 584084b into rollbar:master Nov 19, 2013
@sbezboro
Copy link
Contributor

Awesome, thanks for the PR!

@sbezboro
Copy link
Contributor

Pushed to rubygems as version 0.12.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants