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.

Sign up for GitHub

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

Jump to bottom

Misc improvements #18

Merged
merged 10 commits into from Apr 22, 2015
Merged

Misc improvements #18

merged 10 commits into from Apr 22, 2015

Conversation

gsakkis
Copy link
Contributor

@gsakkis gsakkis commented Apr 19, 2015

  • Fixed threaded_cached_property_with_ttl to actually be thread-safe.
  • Allow the del statement for resetting cached properties with ttl instead of del obj._cache[attr].
  • Overall code and test refactoring.

@gsakkis
Tweak unit tests
7069f84 
- Fix shadowed Testcase class name in test_cached_property_ttl
- Single Testcase class per cached_property variant.
- Make test modules non-executable.
@gsakkis
lint (flake8) fixes
33d3969
@gsakkis
Allow del statement to work for cached properties with ttl
6dfcdec
@gsakkis
Make cached_property thread-safe and alias threaded_cached_property t…
df46e7d 
…o it.

dict.setdefault() is atomic since three years ago; see http://bugs.python.org/issue13521
@gsakkis
Copy link
Contributor Author

gsakkis commented Apr 19, 2015

Looks like TravisCI uncovered a pypy bug; I opened an issue for it here.

@gsakkis
Fix the previous commit and add more test assertions to show why it w…
4d0bc43 
…as wrong.

Although the previous commit correctly cached and returned only the first computed
value (since dict.setdefault() is atomic), the actual computation could be performed
more than once in multithreaded environment, with all but the first computed values
being discarded.
@gsakkis
Fix threaded_cached_property_with_ttl.
9557d08
@pydanny
Copy link
Owner

pydanny commented Apr 20, 2015

I love this pull request, I really do! Do we know when the PyPy error fix will make it to a formal release?

@cfbolz
Copy link

cfbolz commented Apr 20, 2015

As a workaround for the PyPy bug it would be possible to add a __set__ method to the descriptor that then always raises the appropriate error. Then pypy would consider it to be a read write descriptor (but writing would still raise).

@gsakkis
Copy link
Contributor Author

gsakkis commented Apr 20, 2015

@pydanny thanks! I don't know when is PyPy's next release but I could add the __set__ method as @cfbolz suggested if you want to make Travis happy before merging.

@pydanny
Copy link
Owner

pydanny commented Apr 20, 2015

@gsakkis Adding the __set__ method so we pass tests is good. I want to see Travis happy before I consider cutting a new release. 😄

@gsakkis
Beef up and refactor tests (DRY)
0268c61
@gsakkis
Refactor cached_property_with_ttl.
96dcca6 
Cache (value, time) in the object's __dict__ instead of doing an extra lookup
in a '_cache' dict; also avoids the potential name clash with an unrelated
attribute named '_cache'.
@gsakkis
Revert cached_property_with_ttl/threaded_cached_property_with_ttl to …
59a6292 
…settable just like cached_property/threaded_cached_property
@gsakkis
Copy link
Contributor Author

gsakkis commented Apr 21, 2015

It turned out to be somewhat trickier. Before adding the __delete__ method, all cached property variants were non-data descriptors, which happen to be settable implicitly. Now this could be inadvertent as intuitively cached properties shouldn't really be settable, so one could suggest to add a __set__ that raises AttributeError to every class. However the implementation of cached_property and threaded_cached_property decorators relies on being non-data descriptors; they break if they get a __set__ method.

So for the sake of consistency and backwards compatibility, I added a __set__ to cached_property_with_ttl that explicitly allows setting instead of raising an error, so that all cached properties behave the same in this respect.

@gsakkis
Copy link
Contributor Author

gsakkis commented Apr 21, 2015

All tests pass at last! Btw I fixed threaded_cached_property_with_ttl to actually be thread-safe and refactored the tests to be reusable between the various cached property variants.

pydanny added a commit that referenced this pull request Apr 22, 2015
@pydanny
Merge pull request #18 from gsakkis/master
b079c2a 
Misc improvements AWESOME!!!!
@pydanny pydanny merged commit b079c2a into pydanny:master Apr 22, 2015
@pydanny
Copy link
Owner

pydanny commented Apr 22, 2015

I will probably cut a release tomorrow. Right now am working towards an epic Friday deadline.

@gsakkis
Copy link
Contributor Author

gsakkis commented Apr 22, 2015

Great, no rush. If you can wait for a day or two I have another feature PR in mind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@gsakkis @pydanny @cfbolz