Skip to content
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

Fixes for LocalClient bugs: glob in local clients to match cloud clients, reset default storage work on default client #436

Merged
merged 11 commits into from
Aug 22, 2024

Conversation

pjbull
Copy link
Member

@pjbull pjbull commented May 25, 2024

  • glob for local clients should return empty list (to match cloud version) rather than raise exception.
  • Reset default client in reset_default_storage_dir so the new storage dir gets picked up because the default client will have to be recreated

Closes #415
Closes #414

Copy link
Contributor

github-actions bot commented May 26, 2024

@github-actions github-actions bot temporarily deployed to pull request May 26, 2024 00:09 Inactive
@github-actions github-actions bot temporarily deployed to pull request May 26, 2024 03:43 Inactive
Copy link

codecov bot commented May 26, 2024

Codecov Report

Attention: Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.

Project coverage is 93.8%. Comparing base (a23a38c) to head (cf20d47).
Report is 2 commits behind head on master.

Files Patch % Lines
cloudpathlib/local/localclient.py 92.3% 1 Missing ⚠️
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     
Files Coverage Δ
cloudpathlib/local/localclient.py 89.4% <92.3%> (-0.6%) ⬇️

... and 1 file with indirect coverage changes

@pjbull pjbull changed the title Update glob in local clients to match cloud clients Fixes for LocalClient bugs: glob in local clients to match cloud clients, reset default storage work on default client May 26, 2024
@github-actions github-actions bot temporarily deployed to pull request May 26, 2024 04:54 Inactive
@pjbull pjbull requested a review from jayqi May 26, 2024 05:01
HISTORY.md Outdated Show resolved Hide resolved
Comment on lines 53 to 54
# Also reset default client so it gets recreated with new temp dir
cls._default_client = None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@pjbull
Copy link
Member Author

pjbull commented May 28, 2024

Update this PR to do the following:

  • Get rid of _default_storage_temp_dir. Only use the per-instance self._local_storage_dir.
  • Deprecation warning for reset_default_storage_dir
  • New reset_default_client that can be used for test cleanup and clears the ._default_client instance on the class.
  • Update documentation on recommendations for test cleanup.

@github-actions github-actions bot temporarily deployed to pull request August 13, 2024 17:52 Inactive
@github-actions github-actions bot temporarily deployed to pull request August 14, 2024 21:55 Inactive
@github-actions github-actions bot temporarily deployed to pull request August 14, 2024 21:58 Inactive
@github-actions github-actions bot temporarily deployed to pull request August 14, 2024 22:54 Inactive
@pjbull
Copy link
Member Author

pjbull commented Aug 14, 2024

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:

Each LocalClient class has a TemporaryDirectory instance that serves as its default local storage location. This is stored as an attribute of the class so that it persists across client instances. (For real cloud clients, authenticating multiple times to the same storage location doesn't affect the contents.)

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 _local_storage_dir only (no other settings).

@pjbull
Copy link
Member Author

pjbull commented Aug 20, 2024

@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 <[email protected]>
@github-actions github-actions bot temporarily deployed to pull request August 22, 2024 02:21 Inactive
@jayqi jayqi merged commit f3605a6 into master Aug 22, 2024
25 checks passed
@jayqi jayqi deleted the 415-glob-local branch August 22, 2024 04:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants