-
Notifications
You must be signed in to change notification settings - Fork 0
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
added is_cachable to requirements #608
Conversation
Reviewer's Guide by SourceryThis pull request introduces a new File-Level Changes
Tips
|
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 @alimaktabi - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
@@ -55,6 +55,7 @@ class ConstraintVerification(ABC): | |||
_param_keys = [] | |||
app_name = ConstraintApp.GENERAL.value | |||
__response_text = "" |
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: Consider adding a comment explaining the purpose and usage of the is_cachable
attribute
This attribute controls whether the constraint verification result should be cached. It might be helpful to provide examples of when it should be set to False, such as for time-sensitive or security-critical verifications.
cache_data, | ||
caching_time, | ||
) | ||
if constraint.is_cachable: |
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.
issue (complexity): Consider refactoring the caching logic into a separate function.
The new code introduces additional complexity due to the conditional caching logic based on constraint.is_cachable
. This adds cognitive load and can make the code harder to read and maintain. Consider refactoring the caching logic into a separate function to encapsulate the behavior and improve readability. This would reduce duplication and make the main function more straightforward. Here's a suggestion:
def cache_constraint_result(cache_key, is_verified, info):
caching_time = 60 * 60 if is_verified else 60
expiration_time = time.time() + caching_time
cache_data = {
"is_verified": is_verified,
"info": info,
"expiration_time": expiration_time,
}
cache.set(cache_key, cache_data, caching_time)
return cache_data
# In the main function
if constraint.is_cachable:
cache_data = cache_constraint_result(cache_key, is_verified, info)
else:
cache_data = {"is_verified": is_verified, "info": info}
if not cache_data.get("is_verified"):
error_messages[c.title] = constraint.response
result[c.pk] = cache_data
This refactoring should help in making the code cleaner and easier to maintain.
Summary by Sourcery
Add a new 'is_cachable' attribute to constraints to determine if their verification results should be cached, and update the caching logic in validators to respect this attribute.
Enhancements: