-
Notifications
You must be signed in to change notification settings - Fork 14
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
feat: added possibility to toggle background cache system #202
Conversation
Reviewer's Guide by SourceryThis pull request adds the ability to toggle the background cache refresh system. It introduces a new configuration setting, updates the main cache update script to respect this setting, and adds a corresponding test fixture. Sequence diagram for cache update process with togglesequenceDiagram
participant User
participant Script as check_and_update_cache.py
participant Settings
participant Logger
User->>Script: Run main()
Script->>Settings: Check background_cache_refresh_enabled
alt Background cache refresh enabled
Script->>Logger: Log "Starting Redis cache update..."
Script->>Script: get_soon_expired_cache_keys()
else Background cache refresh disabled
Script->>Logger: Log "Background Cache Refresh system is disabled"
Script->>User: Raise SystemExit
end
Class diagram for updated Settings classclassDiagram
class Settings {
+bool background_cache_refresh_enabled
+str api_cache_key_prefix
}
note for Settings "New attribute background_cache_refresh_enabled added to toggle background cache refresh system."
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @TeKrop - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider improving the error handling in the main() function. Raising a SystemExit might be too abrupt; a more graceful shutdown or alternative behavior when background refresh is disabled could be beneficial.
- It would be helpful to add some documentation or comments explaining the purpose and implications of disabling the background cache refresh system, especially in the Settings class where the new flag is introduced.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
@pytest.fixture(autouse=True) | ||
def _set_background_cache_refresh_enabled(): | ||
with patch( | ||
"app.common.cache_manager.settings.background_cache_refresh_enabled", | ||
True, | ||
): | ||
yield |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (testing): New fixture added for background cache refresh setting
This fixture sets the background_cache_refresh_enabled setting to True for all tests in this file. Consider adding tests that cover the case when this setting is False to ensure the new functionality is fully tested.
@pytest.fixture(autouse=True)
def background_cache_refresh_setting(request):
def set_value(value):
with patch("app.common.cache_manager.settings.background_cache_refresh_enabled", value):
yield
if request.param:
yield from set_value(True)
else:
yield from set_value(False)
@pytest.mark.parametrize("background_cache_refresh_setting", [True, False], indirect=True)
Quality Gate passedIssues Measures |
Summary by Sourcery
Add a feature to toggle the background cache refresh system via a configuration setting. Implement a check in the main script to log a warning and exit if the system is disabled. Update tests to ensure the background cache refresh system is enabled during execution.
New Features:
Enhancements:
Tests: