ActiveJob: delegate deserialization to the job class #18260

Merged
merged 1 commit into from Dec 30, 2014

Conversation

Projects
None yet
3 participants
@isaacseymour
Contributor

isaacseymour commented Dec 30, 2014

Renames ActiveJob::Base.deserialize to ActiveJob::Base.instantitate_from_job_data (not convinced this is a good name), and delegates full deserialization to ActiveJob::Base#deserialize. This allows subclasses to override ActiveJob::Base#deserialize (they can already override #serialize) to attach metadata to the job. Without this, the only way to attach metadata to a job and retrieve it is by monkey-patching ActiveJob::Base.deserialize 馃檲

@seuros

This comment has been minimized.

Show comment
Hide comment
Member

seuros commented Dec 30, 2014

cc @dhh

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Dec 30, 2014

Member

Looks good, but I don't think we need to change the class method. It's fine to have the class and instance methods both be called deserialize.

Member

dhh commented Dec 30, 2014

Looks good, but I don't think we need to change the class method. It's fine to have the class and instance methods both be called deserialize.

@seuros seuros added the activejob label Dec 30, 2014

@isaacseymour

This comment has been minimized.

Show comment
Hide comment
@isaacseymour

isaacseymour Dec 30, 2014

Contributor

Updated. ActiveJob specs still pass locally. Shall I squash? Also, is there any chance of this making it onto 4-2-stable? I'm pretty sure it's completely non-breaking.

Contributor

isaacseymour commented Dec 30, 2014

Updated. ActiveJob specs still pass locally. Shall I squash? Also, is there any chance of this making it onto 4-2-stable? I'm pretty sure it's completely non-breaking.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Dec 30, 2014

Member

Please squash. I don't think it's a good idea to apply to stable. It's a feature upgrade.

On Dec 30, 2014, at 11:24, Isaac Seymour notifications@github.com wrote:

Updated. ActiveJob specs still pass locally. Shall I squash? Also, is there any chance of this making it onto 4-2-stable? I'm pretty sure it's completely non-breaking.


Reply to this email directly or view it on GitHub.

Member

dhh commented Dec 30, 2014

Please squash. I don't think it's a good idea to apply to stable. It's a feature upgrade.

On Dec 30, 2014, at 11:24, Isaac Seymour notifications@github.com wrote:

Updated. ActiveJob specs still pass locally. Shall I squash? Also, is there any chance of this making it onto 4-2-stable? I'm pretty sure it's completely non-breaking.


Reply to this email directly or view it on GitHub.

@isaacseymour

This comment has been minimized.

Show comment
Hide comment
@isaacseymour

isaacseymour Dec 30, 2014

Contributor

Cool, squashed 馃拑

Contributor

isaacseymour commented Dec 30, 2014

Cool, squashed 馃拑

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Dec 30, 2014

Member

Just need some docs too. Explain why the change was needed. Maybe give an example of how you'd overwrite the serializers.

Member

dhh commented Dec 30, 2014

Just need some docs too. Explain why the change was needed. Maybe give an example of how you'd overwrite the serializers.

@isaacseymour

This comment has been minimized.

Show comment
Hide comment
@isaacseymour

isaacseymour Dec 30, 2014

Contributor

Added docs and an example (it's pretty in-depth but I can't think of another decent one - shout if you want me to it down).

Not sure where I should explain the change - the ActiveJob CHANGELOG is empty and I can't find another AJ PR with a CHANGELOG entry.

Contributor

isaacseymour commented Dec 30, 2014

Added docs and an example (it's pretty in-depth but I can't think of another decent one - shout if you want me to it down).

Not sure where I should explain the change - the ActiveJob CHANGELOG is empty and I can't find another AJ PR with a CHANGELOG entry.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Dec 30, 2014

Member

You can have the honors of starting the CHANGELOG then :D

Member

dhh commented Dec 30, 2014

You can have the honors of starting the CHANGELOG then :D

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Dec 30, 2014

Member

Just follow the pattern for any of the other frameworks.

Member

dhh commented Dec 30, 2014

Just follow the pattern for any of the other frameworks.

@isaacseymour

This comment has been minimized.

Show comment
Hide comment
@isaacseymour

isaacseymour Dec 30, 2014

Contributor

Done 馃帀

Contributor

isaacseymour commented Dec 30, 2014

Done 馃帀

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Dec 30, 2014

Member

Lovely!

Member

dhh commented Dec 30, 2014

Lovely!

dhh added a commit that referenced this pull request Dec 30, 2014

Merge pull request #18260 from isaacseymour/active-job-delegate-deser鈥
鈥alize

ActiveJob: delegate deserialization to the job class

@dhh dhh merged commit a4d5e83 into rails:master Dec 30, 2014

1 check was pending

continuous-integration/travis-ci The Travis CI build is in progress
Details

robin850 added a commit that referenced this pull request Dec 31, 2014

Tiny follow-up to #18260 [ci skip]
Indent the list content by 4 spaces instead of 2 to match the other
changelog files. Also wrap the lines around 80 chars.

Finally update the documentation example with nit-picky things.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment