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

WIP: Base implementation for scheduled_at filter #44

Closed

Conversation

bruno-oliveira
Copy link

I'm sure I am missing something here and I could use a nice code review/pair programming session on this one!
I'm new to ruby and to rails as well, decided to contribute in the hopes of learning something new and because David and Jason are a great source of inspiration, so, instead of consuming only videos, podcasts and blogs, I've decided to jump into the deep end and contribute some code!

@bruno-oliveira
Copy link
Author

As stated in the description above, I am sure some key things are missing, like how to exactly (and where) to add the filter logic to ensure both queue implementations know of it as expected and maybe also adding some tests....

@rosa
Copy link
Member

rosa commented Feb 12, 2024

Hey @bruno-oliveira, thanks for trying! A couple of comments:

  • Resque by itself doesn't support scheduled jobs, it needs resque-scheduler. There's a separate issue for this: Add support for resque-scheduler #31. Because of this, I think you should remove all the support for this filter from the Resque adapter extension, as there's no point on supporting that filter without supporting scheduled jobs.
  • The diff on ActiveJob::QueueAdapters::SolidQueueExt is very big. I'd expect it to be very small, with just support for this new filter, why did you change the visibility of so many methods?

@rosa rosa closed this Mar 13, 2024
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