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

Fix errors caused in jinja StrictUndefined mode #2571

Merged
merged 7 commits into from
Nov 21, 2024

Conversation

pamelafox
Copy link
Contributor

Fixes #2522

This PR addresses errors that arise when running jinja in StrictUndefined mode (like mypy for Jinja!). There were different ways that I addressed the issues depending on the attribute:

  • extra_css/extra_js: I added these as attributes to the BaseView, since their usage and documentation implies that they should always exist as empty lists.
  • form.csrf_token: This is only defined if the dev is using SecureForm as the base class, which isn't always the case, so I used the pattern of "form.csrf_token is defined and form.csrf_token" which seems to be the recommended way in Jinja to ensure a variable exists before using it.
  • return_url: From what I could tell, this is only passed in sometimes? So I made an if to only use it if it exists. But I might be wrong about that.
  • widget.input_type: Only the classes that output HTML input fields have this defined. I considered adding input_type: Optional[str] = None to all the other classes, but that would have been a larger change, so I added the "is defined" check to templates instead.
  • config.FLASK_ADMIN*: Since these will only be defined by devs who want to use that particular functionality, I used the "is defined" pattern for them.

Generally, for most cases, I ended up using the "is defined" pattern, but if you think there are more situations where .py changes would be warranted, let me know!


Fixes:

* Jinja templates can now be loaded in StrictUndefined mode.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure if 1) this change warranted an entry,
2) this was the correct version header

Copy link
Contributor

Choose a reason for hiding this comment

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

This looks great - that'll be the next version, so perfect :)

@@ -3,6 +3,7 @@

from admin import app
from admin.data import build_sample_db
from jinja2 import StrictUndefined
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not the optimal import sorting, given that "admin" is a local folder, but ruff does not know that. I could use "." to make it clearer, or we could just be okay with it.
This seems to be the only example complex enough to have both an app.py and a folder of other modules.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am personally fine with it. If someone was so inclined, we could probably add some ruff config that would help it sort "correctly" - but I consider it broadly unimportant given the small scope, as you note.

Copy link
Contributor

@samuelhwilliams samuelhwilliams left a comment

Choose a reason for hiding this comment

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

Great work on this - thank you :)

@samuelhwilliams samuelhwilliams merged commit 2389693 into pallets-eco:master Nov 21, 2024
11 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 7, 2024
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.

Multiple issues when using StrictUndefined in app.jinja_env.undefined
2 participants