Skip to content

refactor: Pass *Task directly to runSteps#546

Merged
mrnugget merged 1 commit into
mainfrom
mrn/follow-up-refactor
May 27, 2021
Merged

refactor: Pass *Task directly to runSteps#546
mrnugget merged 1 commit into
mainfrom
mrn/follow-up-refactor

Conversation

@mrnugget

Copy link
Copy Markdown
Contributor

This is a tiny follow-up to #540 and removes the TODO that Adam confirmed in his review.

It simplifies things by passing *Task directly to runSteps.

This is a tiny follow-up to #540 and removes the TODO that Adam
confirmed in his review.
@mrnugget mrnugget requested a review from a team May 26, 2021 15:39

@courier-new courier-new left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That's so much nicer, great call! 😄

@LawnGnome LawnGnome left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If there was ever an argument for loose coupling of task and the executor, it went out the window when we copied damn near every field on the task into the options anyway. Good thinking!

@mrnugget

Copy link
Copy Markdown
Contributor Author

If there was ever an argument for loose coupling of task and the executor, it went out the window when we copied damn near every field on the task into the options anyway. Good thinking!

Yeah, I also think of the executor now more as "the thing that executes tasks" vs. "the thing that executes steps", now with the Coordinator and the TaskBuilder also doing part of the stuff that the executor previously did.

@mrnugget mrnugget merged commit 9096cae into main May 27, 2021
@mrnugget mrnugget deleted the mrn/follow-up-refactor branch May 27, 2021 07:48
scjohns pushed a commit that referenced this pull request Apr 24, 2023
This is a tiny follow-up to #540 and removes the TODO that Adam
confirmed in his review.
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.

4 participants