@activity.defn support and other minor things #16
Conversation
| @@ -346,7 +348,7 @@ async def test_interceptor(client: temporalio.client.Client, worker: Worker): | |||
| await handle.result() | |||
| await handle.cancel() | |||
| # Ignore this error | |||
| with pytest.raises(temporalio.client.RPCError): | |||
There was a problem hiding this comment.
Hehe I am noticing someone is coming around to more frequent relative imports
There was a problem hiding this comment.
Ha, I am actually deviating from the suggestions of the community. In fact, I had to update the README to basically say "We know the Google Style Guide says not to use relatively imported classes/functions, but we are anyways in tests but not library code!"
The main reason is I want the test to be more readable w/ less characters
| async def test_activity_custom_name(client: Client, worker: ExternalWorker): | ||
| @activity.defn(name="my custom activity name!") | ||
| async def get_name(name: str) -> str: | ||
| return f"Name: {activity.info().activity_type}" |
There was a problem hiding this comment.
The fact that we call it name in the decorator but type in the info/protos is maybe sliiiightly confusing.
We're inconsistent with this in a bunch of places in all the SDKs and such currently, but perhaps it'd be best to call everything type while we have the chance? Thoughts? Not the biggest deal in the world.
There was a problem hiding this comment.
As long as we don't use the keyword type, I agree.
There was a problem hiding this comment.
I struggle with these two terms. I also call it activity_type in temporalio.activity.Info. But when it's in a decorator, I need a short non-keyword term, so both type and activity_type fail that test. Java does this as well with their annotation and really, despite the ambiguity, name= just looks better.
| return fn | ||
|
|
||
| # If name option is available, return decorator function | ||
| if name is not None: |
There was a problem hiding this comment.
You should return partial if fn is None, otherwise defn() doesn't work.
There was a problem hiding this comment.
If I did that then defn sans parens wouldn't work. There is no overload that supports defn() from the type perspective, so if they are calling it, they are violating the docs/spec here.
I could hack it to have name have some kind of tertiary value to support defn() but I would rather not support that form of decorator at all. I suppose I can have a runtime check that if both fn and name are None that it's an error.
There was a problem hiding this comment.
WDYM no overload that supports defn()? Both params are optional.
I see no harm in supporting defn(), what are you concerned about?
There was a problem hiding this comment.
Both params are optional in the non-overloaded function. Calling the non-@overload will fail a type checker with:
All overload variants of "defn" require at least one argument
I see no harm in supporting defn(), what are you concerned about?
There is no harm, there's just no benefit. I will add a runtime failure too.
There was a problem hiding this comment.
I am merging this to get started on the rest of the workflow impl, but we can revisit this if needed.
There was a problem hiding this comment.
Oh I missed the overloads there.
Not critical but really don't see a good reason not to support that..
There was a problem hiding this comment.
But I don't see a good reason to support it.
| @@ -86,8 +86,9 @@ def __init__( | |||
| # Confirm name is present | |||
| name: Optional[str] = getattr(activity, "__temporal_activity_name", None) | |||
| if not name: | |||
| fn_name = getattr(activity, "__name__", "<unknown>") | |||
There was a problem hiding this comment.
NIT: I'd not take the name here and instead print the object directly, it might not even be a function.
There was a problem hiding this comment.
I mentioned in the other now-resolved thread, this gives something like <function test_activity_without_decorator.<locals>.say_hello at 0x0000020D5FC33D90> which is rough. That ugly string is why I didn't put the activity in the string in the first place. I settled on name here now.
What was changed
dict-based worker activities to@activity.defndecorator