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

Add explicit base class for ActiveJob jobs #19034

Merged
merged 1 commit into from
Mar 30, 2015

Conversation

jvanbaarsen
Copy link
Contributor

  • Jobs generated should inherent from ApplicationJob
  • ApplicationJob inherents from ActiveJob::Base

Fixes #16976

/cc @tamird @rafaelfranca

@georgeclaghorn
Copy link
Contributor

Any reason not to create ApplicationJob when a new app is first created, like ApplicationController?

@jvanbaarsen
Copy link
Contributor Author

@georgeclaghorn Since not everyone might be using ActiveJob. But maybe it would be better to have it there right from the get go. @rafaelfranca @tamird what do you think?

@egilburg
Copy link
Contributor

I remember at some point there was Rails PR for ApplicationModel that all models inherited from (rather than ActiveRecord::Base, and then it didn't make it for some reason. :-(

@cristianbica
Copy link
Member

app/jobs/application_job.rb should be from the beginning just like application_controller.rb. Also it would be nice if we show a log warning for people that inherit from ActiveJob::Base to switch to ApplicationJob.

@jvanbaarsen
Copy link
Contributor Author

Ok! I'll add it to the app generator :-) and I'll see if I can figure out the warning :)

@jvanbaarsen
Copy link
Contributor Author

@cristianbica Im looking into the warnings, but not really sure how to tackle this. And besides, I checked with controllers. Its not something we do there (You can use a controller without inhereting from ApplicationController, without a warning). What do you think?

@rebyn
Copy link
Contributor

rebyn commented Mar 6, 2015

Im looking into the warnings, but not really sure how to tackle this.

Some really wild thoughts:

  • warning when people upgrade their Rails gem
  • warning when people run their jobs (and check for the class?)

@jvanbaarsen
Copy link
Contributor Author

@cristianbica What do you think?

@cristianbica
Copy link
Member

Yes talked with the rails team and the warning is not something we want. We don't want to prohibit subclassing ActiveJob::Base but we want to encourage people to don't mangle with ActiveJob::Base directly. So there's no need for a warning. We need a changelog and some suggestions in the rails 5 upgrade guide.

@jvanbaarsen
Copy link
Contributor Author

@cristianbica Ok thanks! I'll fix those later tonight.

@jvanbaarsen jvanbaarsen force-pushed the explicit-job-base-class branch 3 times, most recently from 2172a10 to 50ac9f5 Compare March 15, 2015 11:55
@@ -99,7 +99,7 @@ def test_should_not_eager_load_model_for_rake
end

def test_code_statistics_sanity
assert_match "Code LOC: 5 Test LOC: 0 Code to Test Ratio: 1:0.0",
assert_match "Code LOC: 7 Test LOC: 0 Code to Test Ratio: 1:0.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cristianbica Somehow I had to change this line otherwise the tests did not pass. Do you have any idea why that is? It is because I added the application_job.rb file?

Copy link
Member

Choose a reason for hiding this comment

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

probably yes

@jvanbaarsen
Copy link
Contributor Author

@cristianbica What can I do to get this merged?

@seuros seuros added this to the 5.0.0 milestone Mar 23, 2015
@cristianbica
Copy link
Member

Please squash your commits. Otherwise seems good to me.

@rafaelfranca

* Jobs generated now inherent from ApplicationJob
* ApplicationJob inherents from ActiveJob::Base
* Added entry to changelog

Signed-off-by: Jeroen van Baarsen <jeroenvanbaarsen@gmail.com>
@jvanbaarsen
Copy link
Contributor Author

@cristianbica Squashed commits. Waiting for build to be green

@jvanbaarsen
Copy link
Contributor Author

@cristianbica Test are passing, and the commits are squashed. Anything left for me to do?

@matthewd matthewd merged commit 929a794 into rails:master Mar 30, 2015
matthewd added a commit that referenced this pull request Mar 30, 2015
Add explicit base class for ActiveJob jobs
@cristianbica
Copy link
Member

nice. thanks @jvanbaarsen

@jvanbaarsen jvanbaarsen deleted the explicit-job-base-class branch March 30, 2015 12:52
* A generated job now inherents from `app/job/application_job.rb` by default.

*Jeroen van Baarsen*

Copy link
Member

Choose a reason for hiding this comment

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

For the future, this should go on top of the file :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah ok :)

@rafaelfranca rafaelfranca modified the milestones: 5.0.0 [temp], 5.0.0 Dec 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ActiveJob should have an explicit app-specific base class a la ApplicationController
10 participants