-
Notifications
You must be signed in to change notification settings - Fork 144
Add aclose to AsyncNetworkBackend interface #1057
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -97,5 +97,8 @@ async def connect_unix_socket( | |||||
| ) -> AsyncNetworkStream: | ||||||
| raise NotImplementedError() # pragma: nocover | ||||||
|
|
||||||
| async def aclose(self) -> None: | ||||||
| raise NotImplementedError() # pragma: nocover | ||||||
|
|
||||||
|
Comment on lines
+100
to
+102
|
||||||
| async def aclose(self) -> None: | |
| raise NotImplementedError() # pragma: nocover |
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 change creates an asymmetry in the API design. The synchronous NetworkBackend class (line 36) does not have a close method, but now AsyncNetworkBackend has an aclose method. This inconsistency may be confusing for users and library maintainers.
Consider whether:
Note that both NetworkStream (line 21) and AsyncNetworkStream (line 66) have close/aclose methods respectively, so the current pattern is that streams (connections) need cleanup but backends (connection factories) do not.