-
-
Notifications
You must be signed in to change notification settings - Fork 195
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
Move cache factory functionality into the cache backend classes. #219
Move cache factory functionality into the cache backend classes. #219
Conversation
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.
Looks good to me!
"is deprecated. Use the a full path to backend classes " | ||
"directly.", | ||
category=DeprecationWarning) | ||
import_me = type(self).__module__ + '.backends.' + import_me |
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.
type(self).__module__
is breaking any application that subclasses this class since it will then resolve to the module where the subclass is. So I think this should remain as a hardcoded 'flask_caching.backends.' + import_me
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.
This line only runs if the cache backend you specify in your config (in CACHE_TYPE
) doesn’t contain a dot (see line 218). If you define your own backend, it will be of the form your_package.your_module.YourClass
, so import_me
won’t be mangled for you.
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.
i was more thinking about the built-in cache types. CACHE_TYPE = 'null'
for example. Why would I ever want that to resolve to something different than the built-in one?
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.
In that case, null
gets expanded to flask_caching.backends.null
; self.module
is always flask_caching
in this context; itʼs written this way so even if we rename the whole module, we donʼt have to modify it here.
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.
It isn't if you subclass the Caching
class in your own module. Then it resolves to that one :/
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.
Ah, so thatʼs what you meant! Sorry for not getting it earlier.
Yes, you are right. I didnʼt consider that when writing the code because i couldnʼt imagine a use case for that. Unless you want to submit a fix, iʼll look into it later this week.
This is a partial solution to #213