Refactor error handling in CustomDjangoCache #11808
Conversation
github-actions
bot
added
DEV: backend
Build Artifacts
|
|
py 2.7 tests are failing because 0-args super() is supported from py 3.0 |
There was a problem hiding this comment.
We do indeed want this to be Python 2.7 compatible, and most of the instances you have written are.
I have suggested a way to DRY this up slightly, which will also solve this, but you could equally just not call super in that way in the couple of instances you have.
| @@ -13,6 +13,32 @@ class CustomDjangoCache(DjangoCache): | |||
| https://github.com/grantjenks/python-diskcache/blob/v4.1.0/diskcache/djangocache.py | |||
| """ | |||
|
|
|||
| ERRORS_TO_HANDLE = (sqlite3.OperationalError, AssertionError) | |||
|
|
|||
| def try_execute_return_none_on_error(self, method, *args, **kwargs): | |||
There was a problem hiding this comment.
I think you could simplify this all somewhat by having a single method like this:
def try_execute(self, method_name, error_return_value, *args, **kwargs):
try:
method = getattr(super(CustomDjangoCache, self), method_name)
if method is None:
raise ValueError("{method_name} is not a valid method".format(method_name=method_name))
return method(*args, **kwargs)
except self.ERRORS_TO_HANDLE:
return error_return_value
That way you can guarantee you're using the Py2.7 compatible form of super, and you don't need to define 4 different functions that are doing the same thing.
If a function doesn't return a value explicitly in Python, it returns None, so you can pass None as the error_return_value in those cases.
There was a problem hiding this comment.
Thanks for the code snippet, indeed this thought crossed my mind, but i was confused about error_return_value having types. Now it makes more sense to me
|
I've incorporated the suggested changes, let me know for additional adjustments or improvements at your convenience |
| except sqlite3.OperationalError: | ||
| return False | ||
| return self.try_execute( | ||
| method_name="add", |
There was a problem hiding this comment.
Sorry, just one more thing - I think this probably does work to pass positional arguments as named keyword arguments, but I would find it less confusing if method_name and error_return_value were passed just as positional arguments in each instance here, so:
return self.try_execute(
"add",
False,
And so on.
There was a problem hiding this comment.
sure, sorry for the trouble
There was a problem hiding this comment.
This is great, thanks @thesujai. I will hold off on final approval here, as we are only merging critical bug fixes right now into release-v0.16.x in anticipation of our 0.16.0 release.
We will merge once that release has been finalized!
Summary
The new approach consolidates handling of
sqlite3.OperationalErrorandAssertionErroracross multiple cache methods, also it makes theinternal implementation more cleaner without breaking anything
…
References
Fixes #11767
…
Reviewer guidance
Confused about the naming, so i gave functions descriptive names
please take your time to review it, no rush
…
Testing checklist
PR process
Reviewer checklist
yarnandpip)