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

Fix for config not updated in cases where it was already created #383

Merged
merged 3 commits into from
Mar 20, 2018
Merged

Fix for config not updated in cases where it was already created #383

merged 3 commits into from
Mar 20, 2018

Conversation

jcugat
Copy link
Contributor

@jcugat jcugat commented Mar 8, 2018

No description provided.

@codecov
Copy link

codecov bot commented Mar 8, 2018

Codecov Report

Merging #383 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #383   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           9      9           
  Lines         853    855    +2     
  Branches       89     90    +1     
=====================================
+ Hits          853    855    +2
Impacted Files Coverage Δ
aiocache/factory.py 100% <100%> (ø) ⬆️

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 cb2acb1...12e3d20. Read the comment docs.

@@ -212,6 +212,18 @@ def test_set_empty_config(self):
with pytest.raises(ValueError):
caches.set_config({})

def test_set_config_updates_default(self):
Copy link
Member

Choose a reason for hiding this comment

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

Hmmm I think I prefer a more explicit test. Something that checks that after doing a set_config the caches that were in the config don't exist anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated!

@@ -140,6 +140,8 @@ def set_config(self, config):
"""
if "default" not in config:
raise ValueError("default config must be provided")
for config_name in config.keys():
self._caches.pop(config_name, None)
Copy link
Member

Choose a reason for hiding this comment

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

Let's update the docstring with this new behavior, it may have some undesired side effects users should be aware. For example if you call multiple times set_config you will be losing the reference to your caches, so we should at least explain it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a new test. I'm accessing the private variable _caches though, didn't know how to test it without it.

@argaen argaen merged commit c0186d0 into aio-libs:master Mar 20, 2018
@jcugat jcugat deleted the change_default_config branch March 24, 2018 11:35
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