Fixes for LocalClient bugs: glob in local clients to match cloud clients, reset default storage work on default client #436
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #436 +/- ##
========================================
- Coverage 93.9% 93.8% -0.1%
========================================
Files 23 23
Lines 1660 1665 +5
========================================
+ Hits 1559 1563 +4
- Misses 101 102 +1
|
pjbull
changed the title
cloudpathlib/local/localclient.py
Outdated
| # Also reset default client so it gets recreated with new temp dir | ||
| cls._default_client = None |
There was a problem hiding this comment.
I'm not convinced this is the right fix.
It took me a while to remember what reset_default_storage_dir is supposed to do and why, and I had to look at our docs.
It feels weird to me that resetting the storage directory has a side effect of changing the default client. If someone had set a default client with non-default values (e.g., say they set a non-default file_cache_mode), then resetting the default client here would lead to unexpected changes later.
It kind of seems to me like the more sensible design is that the storage directory shouldn't be a class instance at all, and that it should be a client instance attribute (i.e., setting storage directory to a default value should be a method and not a property or attribute). And then for cleanup, rather than a reset_default_storage_dir method, there should be a reset_default_client method.
|
Update this PR to do the following:
|
|
I looked at keeping the property just at the instance level, but we need to track a folder to point new clients to for the reason in the docs:
Because of this, I keep the resetting flow that we have. To fix the issue of the default client having storage stick around after resetting, I updated to reset the default client's |
|
@jayqi I think this is probably the best simple fix at the moment. Fixes both the reported issue and a similar issue in this SO post. |
…462) * Change LocalClient to not explicitly store default storage directory * Remove extraneous file --------- Co-authored-by: Jay Qi <jayqi@users.noreply.github.com>
globfor local clients should return empty list (to match cloud version) rather than raise exception.reset_default_storage_dirso the new storage dir gets picked up because the default client will have to be recreatedCloses #415
Closes #414