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

Allow passing in meta to decorated jobs #892

Merged
merged 1 commit into from Dec 6, 2017

Conversation

Projects
None yet
3 participants
@jlucas91
Contributor

jlucas91 commented Oct 20, 2017

No description provided.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 20, 2017

Coverage Status

Coverage increased (+0.006%) to 89.374% when pulling 94a7798 on jlucas91:add-meta-to-job-decorator into 43c9279 on nvie:master.

coveralls commented Oct 20, 2017

Coverage Status

Coverage increased (+0.006%) to 89.374% when pulling 94a7798 on jlucas91:add-meta-to-job-decorator into 43c9279 on nvie:master.

@jlucas91

This comment has been minimized.

Show comment
Hide comment
@jlucas91

jlucas91 Oct 23, 2017

Contributor

Hi @selwin ! I'd love to do what I can to get this merged in. What's the best way for me to do so? Happy to take another approach if you'd prefer.

Pinging you since I see you on a lot of merges, but LMK if there's someone else I should talk to.

Contributor

jlucas91 commented Oct 23, 2017

Hi @selwin ! I'd love to do what I can to get this merged in. What's the best way for me to do so? Happy to take another approach if you'd prefer.

Pinging you since I see you on a lot of merges, but LMK if there's someone else I should talk to.

Show outdated Hide outdated rq/decorators.py Outdated
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 25, 2017

Coverage Status

Coverage increased (+0.06%) to 89.383% when pulling d8f4168 on jlucas91:add-meta-to-job-decorator into 92c88d3 on nvie:master.

coveralls commented Oct 25, 2017

Coverage Status

Coverage increased (+0.06%) to 89.383% when pulling d8f4168 on jlucas91:add-meta-to-job-decorator into 92c88d3 on nvie:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 25, 2017

Coverage Status

Coverage increased (+0.06%) to 89.383% when pulling d8f4168 on jlucas91:add-meta-to-job-decorator into 92c88d3 on nvie:master.

coveralls commented Oct 25, 2017

Coverage Status

Coverage increased (+0.06%) to 89.383% when pulling d8f4168 on jlucas91:add-meta-to-job-decorator into 92c88d3 on nvie:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 25, 2017

Coverage Status

Coverage increased (+0.06%) to 89.383% when pulling d8f4168 on jlucas91:add-meta-to-job-decorator into 92c88d3 on nvie:master.

coveralls commented Oct 25, 2017

Coverage Status

Coverage increased (+0.06%) to 89.383% when pulling d8f4168 on jlucas91:add-meta-to-job-decorator into 92c88d3 on nvie:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 25, 2017

Coverage Status

Coverage increased (+0.06%) to 89.383% when pulling d8f4168 on jlucas91:add-meta-to-job-decorator into 92c88d3 on nvie:master.

coveralls commented Oct 25, 2017

Coverage Status

Coverage increased (+0.06%) to 89.383% when pulling d8f4168 on jlucas91:add-meta-to-job-decorator into 92c88d3 on nvie:master.

@jlucas91

This comment has been minimized.

Show comment
Hide comment
@jlucas91

jlucas91 Oct 25, 2017

Contributor

@selwin Updated. LMK what you think.

Is the breaking change here on depends_on something we need to be concerned about?

Contributor

jlucas91 commented Oct 25, 2017

@selwin Updated. LMK what you think.

Is the breaking change here on depends_on something we need to be concerned about?

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 25, 2017

Coverage Status

Coverage increased (+0.06%) to 89.383% when pulling ae1592d on jlucas91:add-meta-to-job-decorator into 92c88d3 on nvie:master.

coveralls commented Oct 25, 2017

Coverage Status

Coverage increased (+0.06%) to 89.383% when pulling ae1592d on jlucas91:add-meta-to-job-decorator into 92c88d3 on nvie:master.

@selwin

This comment has been minimized.

Show comment
Hide comment
@selwin

selwin Oct 28, 2017

Collaborator

With regards to your comment about depends_on kwarg, since depends_on already made it into a release version of RQ, do you mind overriding it like this?

depends_on = kwargs.pop('depends_on', None)
if not depends_on: # Fall back to option provided by decorator
    depends_on = self.depends_on

Please also move at_front out of kwargs and into __init__. We don't need to worry about backwards compatibility for this kwarg since it never made it into a release version.

It would be great if you can make this change by Monday. I plan on releasing 0.9.0 on Tuesday.

Collaborator

selwin commented Oct 28, 2017

With regards to your comment about depends_on kwarg, since depends_on already made it into a release version of RQ, do you mind overriding it like this?

depends_on = kwargs.pop('depends_on', None)
if not depends_on: # Fall back to option provided by decorator
    depends_on = self.depends_on

Please also move at_front out of kwargs and into __init__. We don't need to worry about backwards compatibility for this kwarg since it never made it into a release version.

It would be great if you can make this change by Monday. I plan on releasing 0.9.0 on Tuesday.

@jlucas91

This comment has been minimized.

Show comment
Hide comment
@jlucas91

jlucas91 Nov 7, 2017

Contributor

Hey @selwin - Sorry to drop off the map. I'll take a look at this tomorrow AM. Thanks for your help here!

Contributor

jlucas91 commented Nov 7, 2017

Hey @selwin - Sorry to drop off the map. I'll take a look at this tomorrow AM. Thanks for your help here!

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Nov 7, 2017

Coverage Status

Coverage increased (+0.09%) to 89.691% when pulling 99865ef on jlucas91:add-meta-to-job-decorator into e5de3df on nvie:master.

coveralls commented Nov 7, 2017

Coverage Status

Coverage increased (+0.09%) to 89.691% when pulling 99865ef on jlucas91:add-meta-to-job-decorator into e5de3df on nvie:master.

@jlucas91

This comment has been minimized.

Show comment
Hide comment
@jlucas91

jlucas91 Nov 7, 2017

Contributor

@selwin Should be good to go. LMK what you think.

Contributor

jlucas91 commented Nov 7, 2017

@selwin Should be good to go. LMK what you think.

@jlucas91

This comment has been minimized.

Show comment
Hide comment
@jlucas91

jlucas91 Dec 4, 2017

Contributor

@selwin Bump on this - Any chance we could merge?

Contributor

jlucas91 commented Dec 4, 2017

@selwin Bump on this - Any chance we could merge?

@selwin selwin merged commit 34c403e into rq:master Dec 6, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.09%) to 89.691%
Details
@selwin

This comment has been minimized.

Show comment
Hide comment
@selwin

selwin Dec 6, 2017

Collaborator

Thanks!

Collaborator

selwin commented Dec 6, 2017

Thanks!

@jlucas91 jlucas91 deleted the jlucas91:add-meta-to-job-decorator branch Dec 6, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment