Chore/fix type hints and general clarity #57
Conversation
… files - Updated type hints for methods in context, handler, server_context, and service modules to improve type safety and clarity. - Removed unnecessary `# type: ignore` comments where type hints are now correctly specified. - General code cleanup for better readability and maintainability.
…s-and-general-clarity
…tness issues, as the method is not currently utilized elsewhere.
…server_context modules - Updated the __await__ method signatures in RestateDurableFuture and ServerDurableFuture to specify return types as typing.Generator. - Changed the state_keys method return type in ServerInvocationContext to RestateDurableFuture[List[str]] for improved type safety. - Refined the create_df method to handle various notification types and added detailed docstring for clarity. - Ensured consistent handling of notification types across methods in ServerInvocationContext.
ouatu-ro
force-pushed
the
chore/fix-type-hints-and-general-clarity
branch
from
f7ec282 to
06772d8
Compare
There was a problem hiding this comment.
Hey @ouatu-ro,
thanks a lot for taking this chore, very much appreciated.
I'm afraid you'd need to rebase one more time, as i had to make some changes today, as i've discovered some issues and refactored a bit to make the whole futures handling way more robust.
Some of the code that you've annotated doesn't exists anymore.
…s-and-general-clarity
There was a problem hiding this comment.
Really good and helpful work @ouatu-ro, thank you for taking the time to adopt the type annotations etc'.
There are two or 3 comments I've left that i'd kindly like to ask you to address.
And then LGTM!
python/restate/server_context.py
Outdated
| if not self.vm.is_completed(handle): | ||
| await self.create_poll_or_cancel_coroutine([handle]) | ||
| res = self.must_take_notification(handle) | ||
| if res is None or serde is None: | ||
| if res is None or serde is None or not isinstance(res, bytes): |
There was a problem hiding this comment.
Not sure about this check. I'd need to review this in depth, can you please revert this check back for now?
There was a problem hiding this comment.
Currently the function runs serde.deserialize on any object, I made it so it runs deserialisation only on bytes. will rewrite it to this for clarity:
if res is None or serde is None:
return res
if isinstance(res, bytes):
return serde.deserialize(res)
return res
Let me know if you want it changed back, but I think it would be a bug? must_take_notification should never return a JSON, it should only return str, list[str], bytes, None.
Cleared all type hint errors for better clarity.
I have also added an
assertinsidemust_take_notificationto ensure it only returnsbytesorNone.Can you confirm that this is correct? Should it not return
strorlist[str], as those are strictly associated with an invocation ID or a state keys response?