Skip to content
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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

bsmithyman
Copy link
Member

@bsmithyman bsmithyman commented Sep 4, 2018

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?

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
Copy link

codecov bot commented Sep 4, 2018

Codecov Report

Merging #258 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #258   +/-   ##
=======================================
  Coverage   96.24%   96.24%           
=======================================
  Files          16       16           
  Lines        2318     2318           
=======================================
  Hits         2231     2231           
  Misses         87       87
Impacted Files Coverage Δ
properties/extras/task.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 093144b...09fe1bb. Read the comment docs.

Copy link
Contributor

@fwkoch fwkoch left a 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 on kwargs? Might allow for more flexibility, though I guess that could happen before passing into report_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 just
    def report_status(self, progress=None, message=None, **kwargs):
        print(...)
    
    Seems a bit circuitous to have to define your input parameters for report_status on TaskStatus and round-trip them through an unnecessary HasProps instance... I mean, I guess if you are overriding report_status, you can do whatever you want, including ignore TaskStatus. Maybe we want to make the default behaviour less complicated?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants