Build: refactor TaskData #9389
Build: refactor TaskData #9389
Conversation
| build_pk: int = None | ||
| build_commit: str = None | ||
|
|
||
| start_time: timezone.datetime = None |
There was a problem hiding this comment.
we could also default this to now
Currently this class holds any values, defining its attributes dynamically, this makes it hard to know what attributes exactly does this object hold, and others are initialized in several parts of the codebase. So, I'm defining all the attributes in a form of a dataclass, with `slots=True` (so no other attributes can be added by mistake). I'm also checking for possible uninitialized data in the task handlers. Note: Dataclasses require type annotations, this doesn't mean we are using type hints or enforcing them in our codebase. Closes #9364
stsewd
force-pushed
the
refactor-task-data
branch
from
153534e
to
56c5c69
Compare
readthedocs/projects/tasks/builds.py
Outdated
|
|
||
| # Dictionary returned from the API. | ||
| build: dict = field(default_factory=dict) | ||
| # If HTML, PDF, ePub, etc formats were build. |
There was a problem hiding this comment.
| # If HTML, PDF, ePub, etc formats were build. | |
| # If HTML, PDF, ePub, etc formats were built. |
| @@ -119,10 +142,10 @@ def before_start(self, task_id, args, kwargs): | |||
|
|
|||
| # Comes from the signature of the task and it's the only required | |||
| # argument | |||
| version_id, = args | |||
| self.data.version_pk = args[0] | |||
There was a problem hiding this comment.
The old syntax is a little weird, but it lets clear that args only has one item. Not a big deal, tho.
I think black changes it to:
(self.data.version_pk,) = args
to make it a little clearer.
readthedocs/projects/tasks/builds.py
Outdated
| build: dict = field(default_factory=dict) | ||
| # If HTML, PDF, ePub, etc formats were build. | ||
| outcomes: dict = field(default_factory=lambda: defaultdict(lambda: False)) | ||
| # Build data for analytics. |
There was a problem hiding this comment.
| # Build data for analytics. | |
| # Build data for analytics (telemetry). |
readthedocs/projects/tasks/builds.py
Outdated
| version_type = self.data.version and self.data.version.type | ||
| send_external_build_status( | ||
| version_type=self.data.version.type, | ||
| version_type=version_type, |
There was a problem hiding this comment.
IMHO, this usage is a little tricky,
>>> True and "external"
'external'
>>> False and "external"
False
>>> True and "branch"
'branch'
>>>
We will be sending two different types: str and bool (False when we don't have a version). This works only because the function checks explicitly for version_type == EXTERNAL.
There was a problem hiding this comment.
Yea, I don't really follow what the code is doing here. The and in the above logic makes me think we're passing a bool. I agree this could be clearer by just using an if statement.
There was a problem hiding this comment.
We will be sending two different types: str and bool
It will be sending None or string, but I'm changing it to an if
| .. note:: | ||
|
|
||
| This handler is called even if the task has failed, | ||
| so some attributes from the `self.data` object may not be defined. |
There was a problem hiding this comment.
I assume this is the bug that we're actually fixing here? 👍
readthedocs/projects/tasks/builds.py
Outdated
| version_type = self.data.version and self.data.version.type | ||
| send_external_build_status( | ||
| version_type=self.data.version.type, | ||
| version_type=version_type, |
There was a problem hiding this comment.
Yea, I don't really follow what the code is doing here. The and in the above logic makes me think we're passing a bool. I agree this could be clearer by just using an if statement.
Currently this class holds any values,
defining its attributes dynamically,
this makes it hard to know what attributes
exactly does this object hold,
and others are initialized in several parts of
the codebase.
So, I'm defining all the attributes in a form of a dataclass,
with
slots=True(so no other attributes can be added by mistake).I'm also checking for possible uninitialized data in the task handlers.
Note: Dataclasses require type annotations,
this doesn't mean we are using type hints or enforcing them in our codebase.
Closes #9364