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

Compatibility with Rails 5 #174

Merged
merged 1 commit into from
Jan 25, 2016
Merged

Conversation

avokhmin
Copy link
Contributor

According to rails/rails#19295

Fixed:

NoMethodError: undefined method `enqueue' for #ActiveJob::QueueAdapters::ShoryukenAdapter:...

@avokhmin
Copy link
Contributor Author

Mayby I missed something, but works for me.

@phstc
Copy link
Collaborator

phstc commented Jan 21, 2016

hi @avokhmin

Because of the indentation changes, it's kind of hard to see the changes to make it compatible with Rails 5, could you update your PR to only include the changes Shoryuken needs for Rails 5, skipping the indentation changes?

Is your change also compatible with Rails 4?

@avokhmin
Copy link
Contributor Author

Because of the indentation changes, it's kind of hard to see the changes to make it compatible with Rails 5, could you update your PR to only include the changes Shoryuken needs for Rails 5, skipping the indentation changes?

#enqueue and #enqueue_at should not be more as class methods, so, I only removed class << self.

Is your change also compatible with Rails 4?

No( They break the back compatibility (see https://github.com/rails/rails/pull/19295/files#diff-fa86cbc86dd18818def18324430d4396R15 for example)

But we can add hook: call class methods from instance methods.

@nickpearson
Copy link

Add ?w=1 to the URL when you're on the Files tab to see the changes with whitespace ignored:

https://github.com/phstc/shoryuken/pull/174/files?w=1

@gshutler
Copy link
Contributor

To expand on what @avokhmin said, something like this will probably be enough to provide cross-compatibility for Rails 4 and 5:

class ShoryukenAdapter
  class << self
    def instance
      @instance ||= new
    end

    def enqueue(job)
      instance.enqueue(job)
    end

    def enqueue_at(job, timestamp)
      instance.enqueue(job, timestamp)
    end
  end
end

@phstc phstc merged commit 6635277 into ruby-shoryuken:master Jan 25, 2016
@phstc
Copy link
Collaborator

phstc commented Jan 25, 2016

Add ?w=1 to the URL when you're on the Files tab to see the changes with whitespace ignored:

TIL ^ tks @nickpearson

To expand on what @avokhmin said, something like this will probably be enough to provide cross-compatibility for Rails 4 and 5:

@gshutler @avokhmin thank you both! Added to master ⭐ 🍻

@avokhmin
Copy link
Contributor Author

👍

@avokhmin
Copy link
Contributor Author

PS: maybe try to add the shoryuken adapter to the rails?

https://github.com/rails/rails/search?utf8=✓&q=shoryuken
We couldn’t find any code matching 'shoryuken'

@phstc
Copy link
Collaborator

phstc commented Jan 28, 2016

hi @avokhmin interesting. I've just created the issue on the Rails repo, let's see how it goes.

rails/rails#23311

@avokhmin
Copy link
Contributor Author

hi @avokhmin interesting. I've just created the issue on the Rails repo, let's see how it goes.

Perfect! Let's wait that they will say

@Startouf
Copy link

Startouf commented May 4, 2017

Hey @phstc, I believe this broke the compatibility with Shoryuken::Later that was overriding the enqueue_at class instance to provide a means to start jobs after 15minutes

(there : https://github.com/joekhoobyar/shoryuken-later/blob/develop/lib/shoryuken/later/active_job_adapter.rb#L28)

How can I fix SHoryuken::Later to make it work with the new version of Shoryuken ?

joekhoobyar/shoryuken-later#16

@phstc
Copy link
Collaborator

phstc commented May 4, 2017

Hi @Startouf

I think you only need to remove this, as enqueued_at is an instance method now.

cc/ @joekhoobyar

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.

5 participants