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

Move cache factory functionality into the cache backend classes. #219

Merged
merged 3 commits into from Feb 20, 2021

Conversation

gergelypolonkai
Copy link
Collaborator

This is a partial solution to #213

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.009%) to 79.27% when pulling d6c43b5 on gergelypolonkai:remove-init-functions into b5fa578 on sh4nks:master.

Copy link
Collaborator

@sh4nks sh4nks left a 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!

@sh4nks sh4nks merged commit cb8bb61 into pallets-eco:master Feb 20, 2021
"is deprecated. Use the a full path to backend classes "
"directly.",
category=DeprecationWarning)
import_me = type(self).__module__ + '.backends.' + import_me
Copy link
Contributor

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

Copy link
Collaborator Author

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.

Copy link
Contributor

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?

Copy link
Collaborator Author

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.

Copy link
Contributor

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 :/

Copy link
Collaborator Author

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants