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.
|
fwkoch
left a comment
There was a problem hiding this comment.
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_propsonkwargs? Might allow for more flexibility, though I guess that could happen before passing intoreport_status. - Should
TaskStatusbe 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
TaskStatusat 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_statusonTaskStatusand 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?