-
Notifications
You must be signed in to change notification settings - Fork 9
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
Adjust behaviour of task status reporting #258
base: master
Are you sure you want to change the base?
Conversation
The current implementation here is a bit clunky, and I see no particular reason why we can't just take keyword arguments. That means that using it is as simple as `self.report_status(progress=0.5, message='Glass half full')`, instead of the current `self.report_status(dict(progress=0.5, message='Glass half empty'))`.
Codecov Report
@@ Coverage Diff @@
## master #258 +/- ##
=======================================
Coverage 96.24% 96.24%
=======================================
Files 16 16
Lines 2318 2318
=======================================
Hits 2231 2231
Misses 87 87
Continue to review full report at Codecov.
|
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 is fine with me. The advantage of the old way is that status
could have been an instance of TaskStatus
or a dict
or anything else that validates into TaskStatus
; now it is required to be kwargs
(i.e. a dict
). The new way presents a cleaner API, though, by basically disallowing TaskStatus
anywhere except internal to report_status
.
This brings up a series of questions, not entirely related to this specific PR, but related to report_status
:
- Should we
filter_props
onkwargs
? Might allow for more flexibility, though I guess that could happen before passing intoreport_status
. - Should
TaskStatus
be defined on the class, i.e.self.TaskStatus
? Not sure if there's any advantage here, but it does feel a bit more in line with everything else. - Maybe most importantly - Is there any reason to have
TaskStatus
at all...? Why not justSeems a bit circuitous to have to define your input parameters fordef report_status(self, progress=None, message=None, **kwargs): print(...)
report_status
onTaskStatus
and round-trip them through an unnecessary HasProps instance... I mean, I guess if you are overridingreport_status
, you can do whatever you want, including ignoreTaskStatus
. Maybe we want to make the default behaviour less complicated?
The goal here is mainly to make the task status interface a bit less... weird. In the prior incarnation, it takes a dict of options for the status reporting. Really strange. Not sure how we got there (probably me?), but we may as well fix it before we start using this stuff a lot more.
Thoughts @fwkoch?