Add task relationship APIs #822
Conversation
dylanholmes
force-pushed
the
feature/task-relationship-apis
branch
2 times, most recently
from
f061d1e to
8b63baf
Compare
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
| def add_child(self, child: ActionsSubtask) -> ActionsSubtask: | ||
| if child.id not in self.child_ids: | ||
| self.child_ids.append(child.id) | ||
|
|
||
| if self.id not in child.parent_ids: | ||
| child.parent_ids.append(self.id) | ||
|
|
||
| return child | ||
|
|
||
| def add_parent(self, parent: ActionsSubtask) -> ActionsSubtask: | ||
| if parent.id not in self.parent_ids: | ||
| self.parent_ids.append(parent.id) | ||
|
|
||
| if self.id not in parent.child_ids: | ||
| parent.child_ids.append(self.id) | ||
|
|
||
| return parent | ||
|
|
There was a problem hiding this comment.
Is this necessary to remove? I understand it's for consistency, but isn't that what overriding methods is good for? Changing behavior in subclasses?
There was a problem hiding this comment.
Its not necessary to remove. I just thought it makes the logic easier to reason about.
I agree that overriding is for changing behavior in subclasses, but it can be abused. It seems unintuitive to change mutate objects passed in as arguments in a subclass only.
As an extreme example, imagine a class that has a method called sort which returns a list. It would be pretty strange if the base class returned a new list in sorted order, but a subclass sorted in-place and returned a reference to the same list.
That said, this case is not nearly that extreme, and I don't have the strongest opinion. I just gave the example as the intuition behind my position.
Let me know if this changes your opinion at all. I'll gladly revert if you prefer
There was a problem hiding this comment.
I'm sold, thanks for explaining!
dylanholmes
force-pushed
the
feature/task-relationship-apis
branch
3 times, most recently
from
6d28983 to
40eb047
Compare
collindutter
force-pushed
the
feature/task-relationship-apis
branch
from
ea086fb to
0b5bffc
Compare
collindutter
force-pushed
the
feature/task-relationship-apis
branch
from
0b5bffc to
bc924df
Compare
See CHANGLOG.md for the majority of changes and docs + unit tests for example usages.
One change not listed in the change log is the removal of
ActionSubTask.add_child()andActionSubTask.add_parent()due to effectively no change in behavior. The behavior ofActionSubTask.add_child()andActionSubTask.add_parent()has been moved up the hierarchy intoBaseTaskand changed to only define relationships asymmetrically in order to be consistent with the approach of passing parent or child ids intoBaseTask(). This change in behavior required one update toToolKitTask.add_subtaskto invoke bothadd_childandadd_parentsinceStructure.resolve_relationships()only effects tasks that have been added toStructuredirectly (which does not include sub tasks.📚 Documentation preview 📚: https://griptape--822.org.readthedocs.build//822/