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

Use cache as context manager. #1053

Merged
merged 1 commit into from
May 25, 2016
Merged

Use cache as context manager. #1053

merged 1 commit into from
May 25, 2016

Conversation

jgosmann
Copy link
Collaborator

@jgosmann jgosmann commented May 2, 2016

Do not rely on __del__ which can cause problems.

Addresses #1041, #1048.

@tbekolay
Copy link
Member

Pushed a commit to fix two PEP8 issues; this also needs a bugfix changelog entry. Aside from that looks good to me!

@jgosmann jgosmann assigned jgosmann and unassigned jgosmann May 10, 2016
@jgosmann
Copy link
Collaborator Author

Added the changelog entry.

@tbekolay
Copy link
Member

Cool; will merge on a second reviewer's +1.

@jgosmann
Copy link
Collaborator Author

Any second reviewer available?

"Decoder cache could not acquire lock and was deactivated.")
self._index = {}
self.readonly = True
self._index = {}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why does this default to an empty dict, rather than say None? If it were None, then you could just check for that on __exit__, rather than having to use hasattr.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right. I fixed this. I guess, I was worried about having always to check whether it is None in that case, but all the code where it would be necessary doesn't get executed in that case for other reasons.

@hunse
Copy link
Collaborator

hunse commented May 16, 2016

Other than that one little question, it's all fine by me.

@Seanny123
Copy link
Contributor

@hunse just a friendly reminder that this issue appears to be waiting on your OK given Jan's last modification of switching from an empty dict to None (:

@hunse
Copy link
Collaborator

hunse commented May 25, 2016

Oh yeah, that's fine.

Do not rely on __del__ which can cause problems.

Addresses #1041, #1048.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants