-
Notifications
You must be signed in to change notification settings - Fork 44
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
Fix automount deprecations #2860
Conversation
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.
LGTM if you can get the tests to pass without any warnings
@@ -68,7 +68,7 @@ def test_run_class(client, servicer): | |||
assert len(servicer.precreated_functions) == 0 | |||
assert servicer.n_functions == 0 | |||
with app.run(client=client): | |||
method_handle_object_id = Foo.bar.object_id # method handle object id will probably go away | |||
method_handle_object_id = Foo._get_class_service_function().object_id # type: ignore |
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.
Is the subsequent assertion actually doing anything useful with this change?
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.
It does ensure that the underlying class service function has been hydrated by app.run, which we probably test in other places too, but it doesn't hurt to have another assertion about it here I think
@@ -705,7 +705,7 @@ def test_web_cls(client): | |||
assert c.asgi.web_url == "http://asgi.internal" | |||
|
|||
|
|||
handler_app = App("handler-app") | |||
handler_app = App("handler-app", include_source=True) |
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.
Do you intend not to remove this later or did you just get tired of writing the comment everywhere? 😁
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.
It was one of the instances where my regex didn't trigger so I made the edit manually :D
Also fixes a couple of outstanding issues with the
include_source
option not working for:This also sets pending=False on the deprecation warning, so we need to fix internal integration tests before merging this as well.
Check these boxes or delete any item (or this section) if not relevant for this PR.
Note on protobuf: protobuf message changes in one place may have impact to
multiple entities (client, server, worker, database). See points above.
Changelog