Skip to content

Conversation

@skarzi
Copy link

@skarzi skarzi commented Oct 22, 2025

What was changed

Switched to classmethods in all factory methods I found without thoroughly analyzing the entire codebase (please let me know if I missed any factory methods!).
Also fixed some small issues with directly setting defaults to MappingProxyType instances instead of using default_factory.

Why?

Switching to classmethod and cls (where possible) will enhance type safety, support inheritance, and boost overall code maintainability. For example, it will be easier to replace the WorkflowHandle class with Client by simply subclassing Client and overriding the appropriate methods, and Client.connect will still work as intended (currently, it returns the original, temporary Client instance, which makes Client closed to extension).

Checklist

  1. Closes - (no GitHub issue created)

  2. How was this tested:

Tested by running all tests (poe test), linters (poe lint, poe format), and manually testing some changes by running local temporal worker and clients in my private project.

  1. Any docs updates needed?

Nope, this change is backward-compatible.

@skarzi skarzi requested a review from a team as a code owner October 22, 2025 17:32
@CLAassistant
Copy link

CLAassistant commented Oct 22, 2025

CLA assistant check
All committers have signed the CLA.

@tconley1428
Copy link
Contributor

Can you elaborate a bit on the practical impacts here? Is there some specific thing you are trying to do that you can't accomplish?

@skarzi
Copy link
Author

skarzi commented Oct 22, 2025

Can you elaborate a bit on the practical impacts here? Is there some specific thing you are trying to do that you can't accomplish?

@tconley1428, sure.

My concrete use case is to replace the WorkflowHandle class with a custom one that does some custom stuff, e.g., enriching FailureError (and all its subclasses) instances message attribute with the stack trace dump - something similar to the stuff described in this comment.

It's currently impossible to fully replace it, because the client's OutboundInterceptor cannot intercept get_workflow_handle, so I need to use the custom Client class, but because a lot of functions in my project use client_class.connect (they can, because it's a public method on Client class), so I have to copy-paste connect method, just to replace the WorkflowHandle with my custom WorkflowHandle subclass.

Basically, using staticmethod for factory methods is not recommended, because it makes the class totally closed to extension (via inheritance). The method is tightly coupled to the class where it's defined, so when we subclass the class, we need to override the factory method to create instances of the subclass.

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.

3 participants