-
Notifications
You must be signed in to change notification settings - Fork 274
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
create new session if refresh is True #939
base: main
Are you sure you want to change the base?
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.
I have a couple of small comments on wording.
On the kwargs: this is probably a leftover from previous code. It could be removed, but let's not do that in this PR.
s3fs/core.py
Outdated
Examples | ||
-------- | ||
>>> s3 = S3FileSystem(profile="<profile name>") # doctest: +SKIP | ||
>>> s3.set_session() # doctest: +SKIP |
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 method is actually async, so you would only call it with await
within a coroutine, as happens elsewhere in the class. The exact alias is _connect
(with the underscore), whereas connect
(without the underscore) is the blocking version.
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.
Thank you for the clarifications. I haven't directly worked with async
before and overlooked that. Let me know if you have additional suggestions for my edits.
s3fs/core.py
Outdated
@@ -480,9 +480,26 @@ def _prepare_config_kwargs(self): | |||
|
|||
async def set_session(self, refresh=False, kwargs={}): | |||
"""Establish S3 connection object. | |||
|
|||
``connect`` is an alias of ``set_session``. This method called with |
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 method is called by any operation on an s3fs instance.
You might call this directly from async code, so that you can assign the client object to a local variable
(^ this is implied in the documentation on async, but should no longer be important for most people).
I suppose a test would be nice for completeness: call connect(), and assert that the ._s3 and .session attributes have both changed. I wonder if this will cause warnings that those resources weren't closed, or if garbage collection will take care of it. |
One observation is that
kwargs
is an unused argument. Should I remove it? Or do something else to handle it?