-
Notifications
You must be signed in to change notification settings - Fork 63
templates: Be strict about undefined variables #5201
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
Conversation
We shouldn't have any reason to use undefined variables in MAS, so silently printing as empty strings (etc) seems undesirable. This will also be helpful for operators with custom templates, since then they will notice their templates being broken.
Deploying matrix-authentication-service-docs with
|
| Latest commit: |
eeeec35
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://abacde21.matrix-authentication-service-docs.pages.dev |
| Branch Preview URL: | https://rei-templatecheck-stricter.matrix-authentication-service-docs.pages.dev |
templates/components/footer.html
Outdated
| {%- endif -%} | ||
|
|
||
| {%- if branding.tos_uri -%} | ||
| {%- if branding.tos_uri is defined -%} |
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 scares me a little bit, we don't have 100% coverage of templates with the samples, so we might have missed some spots. What about using SemiStrict instead, which allows for if undefined?
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.
Changed my mind on this: how about using is none instead? So we never make fields undefined, just none/nullable. I think this would offer even more typo resistance than is_defined
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.
Agreed on using none instead of undefined, but the point that this PR might break other stuff still stands
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.
Yeah OK, this is going to be gnarly, I've downgraded in 20a13f4
20a13f4 to
eeeec35
Compare
Currently the template engine is lax about undefined variables, meaning typos become silent rendering issues.
By switching to the strict undefined variable behaviour, we can make these issues noisier and easier to notice.
This also makes the
templates checkcommand more useful.