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

Allow to disable rule caching by setting "DIAZO_ALWAYS_CACHE_RULES" t… #245

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thet
Copy link
Member

@thet thet commented Sep 4, 2024

…o a value which evaluates to False.

@gforcada this is not yet finished as this might even be a breaking change. therefore I sumitted this more to discuss than to review.

I was hit today again by this setting.
I have a installation where the setting DIAZO_ALWAYS_CACHE_RULES true is present. I checked parts/instance/zope.conf and set it to False without any effect.
AFAIK there are some environment variable which only need to be defined where the actual value isn't considered. IMO the value should have an effect on the setting.

@mister-roboto
Copy link

@thet thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically.

Happy hacking!

@thet thet marked this pull request as draft September 4, 2024 17:56
@thet thet requested a review from gforcada September 4, 2024 17:56
@thet
Copy link
Member Author

thet commented Sep 4, 2024

@jenkins-plone-org please run jobs

@@ -55,13 +51,13 @@ def develop_theme(self):
return False
if self.debug_theme():
return True
if environ.get("DIAZO_ALWAYS_CACHE_RULES"):
if utils.is_truthy(environ.get("DIAZO_ALWAYS_CACHE_RULES", False)):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^^ This is the actual change.

Copy link
Member

@mauritsvanrees mauritsvanrees left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me this is fine.
Maybe call it a feature.
Does this still need to be Draft?

I am going to make Plone releases today or tomorrow. This could get in. But maybe get one more reviewer.

@mauritsvanrees
Copy link
Member

One Jenkins failure, but it is unrelated: browser cannot be found.

@thet thet marked this pull request as ready for review September 5, 2024 12:43
@@ -43,6 +43,11 @@
LOGGER = logging.getLogger("plone.app.theming")


def is_truthy(value):
"""Return `true`, if value is truthy."""
return value and str(value).lower() in ("1", "y", "yes", "t", "true")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First of all, I would rather check for "falsish" values.

That would handle cases where somebody set DIAZO_ALWAYS_CACHE_RULES to on or enabled.

On the other end, there is already some code doing something similar:

return diazo_debug in ("1", "y", "yes", "t", "true")

Then I would not add a helper function for this.

But if you really have to, avoid to handling types that are not strings or call it with another name... is_truthy(2) will return False which is not immediately clear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants