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

Define Config class inside SidekiqUniqueJobs module #343

Merged
merged 2 commits into from
Nov 8, 2018

Conversation

Slike9
Copy link
Contributor

@Slike9 Slike9 commented Nov 7, 2018

Config class used to be defined in Concurrent::MutableStruct which is external, i.e. defined in other library. I guess, it's not good practice to define your code inside a foreign module.
By the way, the pr also fixes #340.

`Config` class used to be defined in `Concurrent::MutableStruct` which
is external, i.e. defined in other library.
Setup redis-server by declaring it in `services` section in .travis.yml
(https://docs.travis-ci.com/user/database-setup/#redis).
@mhenrixon
Copy link
Owner

Thank you @Slike9, the code you suggests I believe will only work for concurrent ruby versions greater than 1.1.1 so we need to enforce that then or provide a fallback to allow both older and newer versions of concurrent ruby to be used. Very please to see this update in concurrent ruby!! I really did not like the way they wanted me to define the config class originally.

@Slike9
Copy link
Contributor Author

Slike9 commented Nov 8, 2018

@mhenrixon sidekiq-unique-jobs requires

spec.add_dependency 'concurrent-ruby', '~> 1.0', '>= 1.0.5'

concurrent-ruby v1.0.0 already supports the api https://github.com/ruby-concurrency/concurrent-ruby/blob/v1.0.0/lib/concurrent/synchronization/abstract_struct.rb#L139. So, the fix will work for all now supported versions of concurrent-ruby. I've run the tests against concurrent-ruby '1.0.5' and they've passed.

@mhenrixon mhenrixon merged commit ead8be3 into mhenrixon:master Nov 8, 2018
@Slike9 Slike9 deleted the fix-config-class-definition branch November 9, 2018 08:11
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.

concurrent-ruby 1.1.1 is causing this gem to break
2 participants