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

Defer include until after initialization as that is when delayed_job_… #18

Merged
merged 4 commits into from
Jan 29, 2021

Conversation

hoenth
Copy link
Contributor

@hoenth hoenth commented Jan 3, 2021

…active_record now instantiates.

This follows the pattern used by delayed_cron_job gem ( codez/delayed_cron_job#27) to make delayed_job_groups_plugin compatible with delayed_job_active_record v1.4.5

Copy link
Member

@jturkel jturkel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution! A few small suggestions but otherwise looks great.

lib/delayed/job_groups/backend/active_record/railtie.rb Outdated Show resolved Hide resolved
lib/delayed/job_groups/backend/active_record/railtie.rb Outdated Show resolved Hide resolved
lib/delayed/job_groups/backend/active_record/railtie.rb Outdated Show resolved Hide resolved
lib/delayed_job_groups_plugin.rb Outdated Show resolved Hide resolved
@hoenth
Copy link
Contributor Author

hoenth commented Jan 6, 2021

I apologize for the delay. I have made all of the requested updates. Thanks for your thoughtful comments

Copy link
Member

@jturkel jturkel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes. Just one more thing I noticed on re-review.

@@ -11,6 +11,15 @@
require 'delayed/job_groups/yaml_loader'
require 'delayed/job_groups/version'

Delayed::Backend::ActiveRecord::Job.send(:include, Delayed::JobGroups::JobExtensions)
if defined?(Delayed::Backend::ActiveRecord)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The gem has a dependency on delayed_job_active_record so won't the Delayed::Backend::ActiveRecord constant always be defined? Seems like this conditional could mask other issues and cause the gem to silently not load properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are very likely correct. I was following the pattern from the delayed_cron gem and didn't investigate why they required that check. Seems having it fail at load makes more sense than having it fail later when an attempt is made to use the group function. I will remove that if statement wrapper.

@kphelps
Copy link
Contributor

kphelps commented Jan 28, 2021

@hoenth any plans to revisit this? I'd love to see it merged!

@will89 will89 merged commit bf02ac1 into salsify:master Jan 29, 2021
@will89
Copy link
Contributor

will89 commented Jan 29, 2021

@hoenth Thanks for this contribution!

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.

None yet

4 participants