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

Change LocalClient to not explicitly store default storage directory #462

Merged
merged 2 commits into from
Aug 22, 2024

Conversation

jayqi
Copy link
Member

@jayqi jayqi commented Aug 22, 2024

This is my proposed alternative fix for #414. This PR is into the branch for #436.

This changes the way that LocalClient stores and accesses what the local storage directory should be. In the default case where no value for local_storage_dir is provided, accessing local storage will call the class method get_default_storage_dir to locate local storage.

This means that calling reset_default_storage_dir will instantly reset the local storage for all clients that are using the default storage, not just future new ones that are created, and not with a special case for the default client.

This feels like a more intuitive fix to me than having the special case for the default client. This means that calling reset_default_storage_dir will have the same effect as deleting all of the files inside the directory, in the sense that all instantiated clients using that storage directory get affected.

@jayqi jayqi requested a review from pjbull August 22, 2024 00:32
Copy link
Contributor

github-actions bot commented Aug 22, 2024

@github-actions github-actions bot temporarily deployed to pull request August 22, 2024 00:34 Inactive
assert not p1.exists()
assert not p2.exists()
Copy link
Member Author

@jayqi jayqi Aug 22, 2024

Choose a reason for hiding this comment

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

This test had failed with my new implementation, but I think the old behavior is not desirable. In the old version of this test, we reset the storage directory before instantiating p2 and writing out p1, which means p2 uses a new default local storage but p1 still uses the old default local storage.

Copy link

codecov bot commented Aug 22, 2024

Codecov Report

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

Project coverage is 93.8%. Comparing base (417fa2a) to head (729d6e7).

Files Patch % Lines
cloudpathlib/local/localclient.py 90.0% 1 Missing ⚠️
Additional details and impacted files
@@               Coverage Diff                @@
##           415-glob-local    #462     +/-   ##
================================================
+ Coverage            93.6%   93.8%   +0.1%     
================================================
  Files                  23      23             
  Lines                1661    1664      +3     
================================================
+ Hits                 1556    1562      +6     
+ Misses                105     102      -3     
Files Coverage Δ
cloudpathlib/local/localclient.py 89.4% <90.0%> (-0.7%) ⬇️

... and 2 files with indirect coverage changes

Copy link
Member

@pjbull pjbull left a comment

Choose a reason for hiding this comment

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

Looks great, thanks!

test.py Outdated Show resolved Hide resolved
@github-actions github-actions bot temporarily deployed to pull request August 22, 2024 01:58 Inactive
@jayqi jayqi merged commit cf20d47 into 415-glob-local Aug 22, 2024
25 checks passed
@jayqi jayqi deleted the 416-glob-local-default-storage branch August 22, 2024 02:20
jayqi added a commit that referenced this pull request Aug 22, 2024
…nts, reset default storage work on default client (#436)

* update glob behavior and test

* Typing and changelog

* no override

* Self typing import

* Simpler fix

* Fix 414

* message for 414

* Change reset path for default

* Fix linting

* More waiting styles

* Change LocalClient to not explicitly store default storage directory (#462)

* Change LocalClient to not explicitly store default storage directory

* Remove extraneous file

---------

Co-authored-by: Jay Qi <[email protected]>

---------

Co-authored-by: Jay Qi <[email protected]>
Co-authored-by: Jay Qi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants