Added capability to pass client session in API #12
Conversation
hellysmile
left a comment
hellysmile
left a comment
There was a problem hiding this comment.
What happens on close? Can session be closed multiple times?
aioslacker/__init__.py
Outdated
| loop=self.loop, | ||
| ), | ||
| ) | ||
| if not isinstance(session, aiohttp.ClientSession): |
There was a problem hiding this comment.
This check makes no sense, as well can hard to debug, just remove this check, please.
There was a problem hiding this comment.
if session is None, then create default session
aioslacker/__init__.py
Outdated
| if not isinstance(session, aiohttp.ClientSession): | ||
| session = aiohttp.ClientSession( | ||
| connector=aiohttp.TCPConnector( | ||
| use_dns_cache=False, |
There was a problem hiding this comment.
Can You remove use_dns_cache=False, as well, it is not needed anymore
aioslacker/__init__.py
Outdated
|
|
||
| super().__init__(token=None, timeout=timeout, loop=loop) | ||
| super().__init__( | ||
| token=None, timeout=timeout, session=session, loop=loop |
There was a problem hiding this comment.
Can You place them one by one on each new line?
|
It seems, if session is custom we should not close it |
|
Can we update upstream slacker dependency pin if we started to support custom sessions? |
Yes, it will be closed only once.
It is a good point, seems like most client libs like aioga , aiodocker close provided session anyway. But from user perspective i guess yes it will be more obvious not to close it. |
aioslacker/__init__.py
Outdated
|
|
||
| if session is None: | ||
| session = aiohttp.ClientSession( | ||
| connector=aiohttp.TCPConnector( |
There was a problem hiding this comment.
connector is not needed here
Seems like yes, tested on example snippet and few others. Seems like works fine with legacy tokens. |
|
hey, can anyone can merge this? |
3 participants
No description provided.