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

Improvement/flexible task iteration #352

Merged
merged 11 commits into from
Sep 20, 2023
Merged

Conversation

essweine
Copy link
Contributor

@essweine essweine commented Sep 6, 2023

This PR includes

  • a rewrite of task iteration to make it more extensible: it was moved out of the Task class and better filtering capabilities were added
  • duplicative workflow methods that returned specific types of tasks were removed and Workflow.get_tasks now takes other keyword arguments besides the state and filters them all at once, in one pass through the tree
  • duplicative task methods were removed
  • some previously private methods are now public
  • translations between task state names and values is now handled by the TaskState class, which was moved into the SpiffWorkflow.utils.task module.
  • documented public methods in Task and Workflow

@essweine essweine marked this pull request as draft September 20, 2023 15:15
Copy link
Collaborator

@danfunk danfunk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The introduction of get_next_task is very helpful.
TaskFilter is a little more verbose, but also a clean and consistent replacement for many alternate ways to get at things - so it definitely reduces cognitive load.
Things like "_is_decendant_of" are much better as public methods.
I really wish that both get_task and get_task_iterator did not both exist. This creates
new cognitive load.

@@ -216,7 +246,7 @@ def do_engine_steps(self, will_complete_task=None, did_complete_task=None):
def update_workflow(wf):
count = 0
# Wanted to use the iterator method here, but at least this is a shorter list
for task in wf.get_tasks(TaskState.READY):
for task in wf.get_tasks(state=TaskState.READY):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want get_tasks to remain available, or are we using get_tasks_iterator everywhere now? It seems to jump back and forth.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are one or two places where it doesn't work. I am hoping as some of the core library task state change methods are cleaned up more, it will be possible to use the iterator exclusively.

@@ -77,8 +77,8 @@ def create_workflow(self, my_task):
def start_workflow(self, my_task):
subworkflow = my_task.workflow.top_workflow.get_subprocess(my_task)
self.copy_data(my_task, subworkflow)
for child in subworkflow.task_tree.children:
child.task_spec._update(child)
start = subworkflow.get_next_task(spec_name='Start')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so so so much cleaner. What an improvement!

@essweine
Copy link
Contributor Author

Also, both methods (get_tasks and get_tasks_iterator) already existed.

The advantage of task filter is that you can extend it if you have custom properties and use your own instead (so for example, you could write a filter in arena's backend that includes spiff properties and pass that in). I added this because there have been many times when I've been hampered by the builtin iterator's capabilities.

@essweine essweine marked this pull request as ready for review September 20, 2023 16:01
@essweine essweine merged commit 90159bd into main Sep 20, 2023
@essweine essweine deleted the improvement/flexible-task-iteration branch September 20, 2023 16:01
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