-
-
Notifications
You must be signed in to change notification settings - Fork 63
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
Refactor the code to make task functions simple functions #15
Conversation
839156d
to
3a93c86
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks nice but, it means we can't access information on the current task anymore, right ? Like if we want to add the current task id in the log context or anything ?
(This may be solved later, I'm not against having an unstable API while we iterate)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I share you concerns @ewjoachim (about accessing current task information).
Nevertheless I like what has been made here 👍
Ok, let's merge it then, and we'll see later how we want to access current task info. Maybe we will add a parameter to the task decorator to decide if we want to pass this or not... In a later PR :D |
Hoping it's ok with everyone, I'll merge now, because I'd like to have a go at some tickets today. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR makes the
manager.task()
method a true decorator by not altering the decorated function (except the addition of adefer()
method to it to replicate the previous behavior).What this allows is the ability to start decorating existing functions without modifying their signature (no
task_run
anymore) and leave them unit testable without relying on the internals ofcabbage
.This code came up when trying to address #3 and I think if we go the route of this PR this makes the implementation of test specific tools in
cabbage
mostly unnecessary.To sum it up, we can now have something like this: