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

Move _handle attribute up in CloudPath __init__ #300

Merged
merged 2 commits into from Dec 15, 2022
Merged

Move _handle attribute up in CloudPath __init__ #300

merged 2 commits into from Dec 15, 2022

Conversation

bryanwweber
Copy link
Contributor

The _handle attribute needs to be set before any code can raise an exception. This is because if something tries to call del after the exception as the process is exiting, _handle must be set to avoid an AttributeError in del.

Resolves #299.

The _handle attribute needs to be set before any code can raise an exception. This is because if something tries to call __del__ after the exception as the process is exiting, _handle must be set to avoid an AttributeError in __del__.

Resolves #299.
@pjbull
Copy link
Member

pjbull commented Dec 15, 2022

Thanks @bryanwweber—looks like you need to run black to format those lines (you can use make format on your local branch).

@bryanwweber
Copy link
Contributor Author

Thanks @pjbull! Fixed now 😄 That's what I get for trying to make the change on the Web interface 😝

@codecov-commenter
Copy link

codecov-commenter commented Dec 15, 2022

Codecov Report

Merging #300 (9a397a6) into master (31f93ac) will decrease coverage by 0.3%.
The diff coverage is 100.0%.

Additional details and impacted files
@@           Coverage Diff            @@
##           master    #300     +/-   ##
========================================
- Coverage    94.9%   94.5%   -0.4%     
========================================
  Files          21      21             
  Lines        1341    1341             
========================================
- Hits         1273    1268      -5     
- Misses         68      73      +5     
Impacted Files Coverage Δ
cloudpathlib/cloudpath.py 93.7% <100.0%> (ø)
cloudpathlib/s3/s3client.py 92.9% <0.0%> (-2.7%) ⬇️
cloudpathlib/gs/gsclient.py 92.8% <0.0%> (-1.8%) ⬇️

@pjbull pjbull changed the base branch from master to 300-local-merge December 15, 2022 21:34
@pjbull
Copy link
Member

pjbull commented Dec 15, 2022

Thanks @bryanwweber! I'll merge this in to a repo-local branch so we can run the live tests.

@pjbull pjbull merged commit 732070b into drivendataorg:300-local-merge Dec 15, 2022
@bryanwweber
Copy link
Contributor Author

Thanks! Do I need to do anything else?

@pjbull
Copy link
Member

pjbull commented Dec 15, 2022

Nope, once tests pass in #301 I'll merge it in! It'll be in the next release, thanks!

@bryanwweber
Copy link
Contributor Author

Great thank you for the quick responses!

pjbull added a commit that referenced this pull request Dec 15, 2022
* Move _handle attribute up in CloudPath __init__

The _handle attribute needs to be set before any code can raise an exception. This is because if something tries to call __del__ after the exception as the process is exiting, _handle must be set to avoid an AttributeError in __del__.

Resolves #299.

* Formatting

Co-authored-by: Bryan Weber <bweber@rebelliondefense.com>

Co-authored-by: Bryan Weber <bryan.w.weber@gmail.com>
Co-authored-by: Bryan Weber <bweber@rebelliondefense.com>
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.

self._handle is not defined in __del__
4 participants