Add a get_token method #6
Conversation
Adds a LRU cache for tokens
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6 +/- ##
==========================================
+ Coverage 96.26% 98.87% +2.60%
==========================================
Files 4 4
Lines 107 177 +70
==========================================
+ Hits 103 175 +72
+ Misses 4 2 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
briehl
left a comment
briehl
left a comment
There was a problem hiding this comment.
A couple comments and suggestions.
src/kbase/auth/_async/client.py
Outdated
| base_url - the base url for the authentication service, for example | ||
| https://kbase.us/services/auth | ||
| cache_max_size - the maximum size of the token and user caches. |
There was a problem hiding this comment.
What happens if it goes over? I get it's an LRU cache but it's worth putting the delete policy in the docstring.
| self._token_url = base_url + "api/V2/token" | ||
| self._me_url = base_url + "api/V2/me" | ||
| if cache_max_size < 1: | ||
| raise ValueError("cache_max_size must be > 0") |
There was a problem hiding this comment.
FWIW, cacheout.LRUCache is fine with 0. That just removes them when they age out.
But it really doesn't matter, IMO.
There was a problem hiding this comment.
yeah, I don't want to allow infinite cache size
| if tk: | ||
| return tk | ||
| if on_cache_miss: | ||
| on_cache_miss() |
There was a problem hiding this comment.
What's the use case for on_cache_miss? Show a warning or something? Or testing?
There was a problem hiding this comment.
Right now just testing, but could be used for stats or something
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_get_token_cache_evict_on_time(auth_users): |
There was a problem hiding this comment.
The duplicate test code here embedded in a single test makes my brain itch.
It might work better with the sync assertion pieces in a separate function.
But, admittedly, I don't have strong feelings.
There was a problem hiding this comment.
yeah, I feel you and went back and forth on this myself. In the end I decided I wanted to make it very clear that the sync and async versions of the code needs to be tested equivalently and make it obvious that that's happening
Adds a LRU cache for tokens