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

Simplify TaskPoller's WorkflowTaskHandler #5901

Merged

Conversation

stephanos
Copy link
Contributor

@stephanos stephanos commented May 10, 2024

What changed?

The TaskPoller we use in the tests takes a workflowTaskHandler which has 5 parameters. But all of them come from PollWorkflowTaskQueueResponse anyway - so might as well use that directly.

Why?

Shortens the function's interface.

And most handlers don't use any of the parameters anyway.

How did you test it?

Is nothing but tests.

Potential risks

Documentation

Is hotfix candidate?

@stephanos stephanos force-pushed the simplify-task-poller-wft-handler branch from 4c7b2c4 to 522f5d2 Compare May 10, 2024 03:10
@stephanos stephanos force-pushed the simplify-task-poller-wft-handler branch 2 times, most recently from d973b97 to 2b440e4 Compare May 10, 2024 03:32
@stephanos stephanos force-pushed the simplify-task-poller-wft-handler branch from 2b440e4 to 8f39005 Compare May 10, 2024 03:37
@stephanos stephanos marked this pull request as ready for review May 10, 2024 03:52
@stephanos stephanos requested a review from a team as a code owner May 10, 2024 03:52
Copy link
Contributor

@dandavison dandavison left a comment

Choose a reason for hiding this comment

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

Nice cleanup!

@stephanos stephanos enabled auto-merge (squash) May 10, 2024 14:44
@stephanos stephanos merged commit 6076b77 into temporalio:main May 10, 2024
42 checks passed
@stephanos stephanos deleted the simplify-task-poller-wft-handler branch May 10, 2024 14:45
Copy link
Member

@alexshtin alexshtin left a comment

Choose a reason for hiding this comment

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

👍 I hope you used global replace for this.

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.

None yet

3 participants