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

Allow symbol as a queue name in shoryuken_options #177

Merged
merged 1 commit into from
Jan 29, 2016
Merged

Allow symbol as a queue name in shoryuken_options #177

merged 1 commit into from
Jan 29, 2016

Conversation

serihiro
Copy link
Contributor

What does this PullRequest change?

This PullRequest allows to use symbol for queue name in shoryuken_options.

e.g.

class SymbolQueueTestWorker
  include Shoryuken::Worker

  shoryuken_options queue: :default
end

In this case, :default will be converted to 'default', and work.

Why is this need?

To use Amazon SQS, I was about to convert a sidekiq worker class to a shoryuken worker class.
But my worker was not be handled by shoryuken worker process though I enqueued to SQS with valid format message...

The converted shoryuken worker class is like the following:

class WhatOneceWasSidekiqWorker
  include Shoryuken::Worker

  shoryuken_options queue: :myworker
end

And I noticed that sidekiq worker accepts symbol queue name, but shoryuken does not.

But as far as I read shoryuken's codes , I could not find any special reasons that Shoryken has to force the queue name to be Symbol.

So if there are not any special reasons, I think it is better to allow the Symbol for portability from Sidekiq.

@serihiro
Copy link
Contributor Author

https://travis-ci.org/phstc/shoryuken/jobs/105426349 ruby2.2.0 build is failed.
I think this is same problem as this #176.

phstc added a commit that referenced this pull request Jan 29, 2016
Allow symbol as a queue name in shoryuken_options
@phstc phstc merged commit 9f9ceed into ruby-shoryuken:master Jan 29, 2016
@phstc
Copy link
Collaborator

phstc commented Jan 29, 2016

Tks @serihiro! Great fix 🍺 , I've also merged #176 in.

@serihiro
Copy link
Contributor Author

@phstc Thank you for reviewing and merging! 🍻

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.

2 participants