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

Simplify cache namespace and key encoding logic for v1 #670

Merged
merged 36 commits into from
Feb 27, 2023

Conversation

padraic-shafer
Copy link
Contributor

What do these changes do?

  • All logic to select the namespace for a cache key, and encode the resulting namespaced key are now encapsulated in the build_key() member of BaseCache and its backend subclasses.
  • Simplifies the burden of creating a custom key_builder for a cache.
  • Consistent use of namespaced keys across backends and helpers (e.g., locks) while still allowing customization of the formatting and encoding of the namespaced key.

Are there changes in behavior for the user?

  • When creating a cache object with a custom key_builder argument...
    • You no longer need to consider the backend encoding requirements for key identifiers; these are handled by the backend cache class.
    • The namespace argument passed to your custom key_builder has already been resolved by the cache: It will be the namespace that was explicitly requested by a client (if requested); otherwise it will be the default namespace that was used to instantiate the cache.
  • If your code was calling the private _build_key() member of a cache, replace those calls with build_key().

Related issue number

Resolves issue #669.

Checklist

  • [X ] I think the code is well written
  • [X ] Unit tests for the changes exist
  • [X ] Documentation reflects the changes
  • [X ] Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> (e.g. 588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the PR
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: Fix issue with non-ascii contents in doctest text files.

@codecov
Copy link

codecov bot commented Feb 19, 2023

Codecov Report

Merging #670 (3d50611) into master (7c80a0f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 3d50611 differs from pull request most recent head 23cd481. Consider uploading reports for the commit 23cd481 to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #670   +/-   ##
=======================================
  Coverage   99.67%   99.67%           
=======================================
  Files          35       35           
  Lines        3743     3747    +4     
=======================================
+ Hits         3731     3735    +4     
  Misses         12       12           
Flag Coverage Δ
unit 99.67% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
aiocache/__init__.py 83.33% <100.00%> (ø)
aiocache/backends/memcached.py 100.00% <100.00%> (ø)
aiocache/backends/memory.py 100.00% <100.00%> (ø)
aiocache/backends/redis.py 99.14% <100.00%> (-0.03%) ⬇️
aiocache/base.py 99.23% <100.00%> (-0.04%) ⬇️
aiocache/decorators.py 100.00% <100.00%> (ø)
aiocache/lock.py 100.00% <100.00%> (ø)
tests/acceptance/test_decorators.py 100.00% <100.00%> (ø)
tests/acceptance/test_lock.py 100.00% <100.00%> (ø)
tests/ut/backends/test_memcached.py 100.00% <100.00%> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 975fb35...23cd481. Read the comment docs.

aiocache/base.py Outdated Show resolved Hide resolved
aiocache/base.py Outdated Show resolved Hide resolved
aiocache/backends/redis.py Outdated Show resolved Hide resolved
aiocache/base.py Outdated Show resolved Hide resolved
@Dreamsorcerer
Copy link
Member

Would also be nice to add type annotations to the methods that have been edited/added (and the key_builder parameter), to get some movement in that direction.

@padraic-shafer
Copy link
Contributor Author

Would also be nice to add type annotations to the methods that have been edited/added (and the key_builder parameter), to get some movement in that direction.

Sure. That should be a simple improvement.

aiocache/base.py Outdated Show resolved Hide resolved
aiocache/base.py Outdated Show resolved Hide resolved
@padraic-shafer
Copy link
Contributor Author

I added the changes we discussed, and started on the type annotations. I could continue adding type annotations in these files, but would like to get your feedback before proceeding further.

aiocache/base.py Outdated Show resolved Hide resolved
@Dreamsorcerer
Copy link
Member

I added the changes we discussed, and started on the type annotations. I could continue adding type annotations in these files, but would like to get your feedback before proceeding further.

Let's keep the changes to a minimum for this PR, further type annotations can be done in another PR. However, I suspect that completing some of the 1.0 refactoring tasks first will make it easier.

aiocache/base.py Outdated Show resolved Hide resolved
aiocache/base.py Outdated Show resolved Hide resolved
aiocache/base.py Outdated Show resolved Hide resolved
@padraic-shafer
Copy link
Contributor Author

Let's keep the changes to a minimum for this PR, further type annotations can be done in another PR. However, I suspect that completing some of the 1.0 refactoring tasks first will make it easier.

Yeah, I agree.

aiocache/base.py Outdated Show resolved Hide resolved
aiocache/base.py Outdated Show resolved Hide resolved
aiocache/factory.py Outdated Show resolved Hide resolved
tests/ut/conftest.py Outdated Show resolved Hide resolved
@padraic-shafer
Copy link
Contributor Author

It looks like the codecov fails are limited to the "if TYPE_CHECKING" conditional imports.

@Dreamsorcerer Dreamsorcerer merged commit 2df2056 into aio-libs:master Feb 27, 2023
@Dreamsorcerer
Copy link
Member

Dreamsorcerer commented Feb 27, 2023

Thanks. If you want to work on any more, I think #676 and #625 are probably good next issues.

@Dreamsorcerer
Copy link
Member

#677 as well. Should be easier to get some typing working then.

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