-
Notifications
You must be signed in to change notification settings - Fork 5
Implement workflow & activity registry #14
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
Conversation
cadence/worker/_registry.py
Outdated
| def workflow( | ||
| self, | ||
| func: Optional[Callable] = None, | ||
| **kwargs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: : Unpack[RegisterWorkflowOptions] to signify the typing of kwargs
cadence/worker/_registry.py
Outdated
| def activity( | ||
| self, | ||
| func: Optional[Callable] = None, | ||
| **kwargs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: : Unpack[RegisterActivityOptions] to signify the typing of kwargs.
| import os | ||
|
|
||
| # Add the project root to the path | ||
| sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is weird
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, we don't need that thanks to uv and pytest
| assert func() == "test" | ||
|
|
||
|
|
||
| if __name__ == "__main__": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
| def test_func(): | ||
| return "decorator_with_options" | ||
|
|
||
| assert "custom_name" in reg._activities |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: No need to rewrite all this, but in general it would be better to assert that get_activity or get_workflow would raise an Exception over checking the backing fields. That way our tests are written against the type's interface instead of its internal details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thanks
What changed?
Implement workflow & activity registry to run workflow/activity
Why?
We need the registry to register workflow/activity to server in order to run it.
How did you test it?
unit test pass:
uv run pytest tests/cadence/worker/test_registry.py -vPotential risks
Release notes
Documentation Changes