This repository has been archived by the owner on Dec 5, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Task class #43
Task class #43
Changes from 22 commits
0c01047
cd5c1ed
a4ad49d
5306265
e2c826c
4bb451f
b931867
fe51ccd
fabc6b8
885474b
ffd4d02
32dfd4d
59a331c
da6f941
aaf0ec7
5cec1a8
985477e
8ccce48
e213454
b61fdca
599e3a2
0328c8e
10fb40f
3f21e67
3eaf0b9
6812428
9da9121
518d829
f6a6dd0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
It seems weird to me to default these things. Shouldn't they be required parameters on any subclasses? That said, validating that they are set on subclasses likely would require using the
__new__
method on a custom metaclass. Perhaps we make an issue to follow up on this instead of trying to resolve it now?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.
They are static attributes and this was the easiest way to define them, but yes they should be required. Every base class should define them, so open to options on how to better do that.
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.
You have an incompatibility here with workflow chaining. You should use
self._payload.process
instead.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.
It might be worth adding a workflow chaining test case.
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.
Updated.
Test for workflow chaining test case sounds like a good idea. Not it.