-
Notifications
You must be signed in to change notification settings - Fork 527
Async connect context manager support - approach 2 - aenter refactored into separate prepare method. #2613
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
Co-authored-by: Berkay Öztürk <info@berkayozturk.net> (cherry picked from commit 87c623e)
…er for async with SessionWithProxy
…ted method to prepare connection for entering its context.
| def _prepare_aenter(self) -> None: | ||
| """ | ||
| All connection changes done before entering connection context have to be done here, as we expose the same api through snowflake.connector.aio.connect() and call this function there at __aenter__ as well. | ||
| """ | ||
| pass |
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 it needed?
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.
That was the clue of this (second) approach - to prevent future differences between with SnowflakeConnection() vs with connect(). But I think I am more in favour of the first one so I am closing this today, unless any objections are there.
| self.__wrapped__ = SnowflakeConnection.__init__ | ||
| self.__name__ = "connect" | ||
| self.__doc__ = SnowflakeConnection.__init__.__doc__ | ||
| self.__module__ = __name__ | ||
| self.__qualname__ = "connect" | ||
| self.__annotations__ = getattr( | ||
| SnowflakeConnection.__init__, "__annotations__", {} | ||
| ) |
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.
All of those are by default reassigned when using @wraps (in update_wrapper)
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.
Correct - I leaned toward this first approach proposal so I have addressed it there - ptal
#2614
Approach 2 - not idempotent aenter but separate prepare method.
Main benefit - not relying on conn.is_closed() in SnowflakeConnection.aenter (no unnecessary logic there).
Main drawback:
Analogous to: https://github.com/aio-libs/aiohttp/blob/master/aiohttp/client.py#L1376-L1412