-
Notifications
You must be signed in to change notification settings - Fork 168
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
Use Jinja for templating #892
Conversation
This is ready, but I'm inclined to leave it outside of this release in case it breaks something. We have plenty of changes anyway. Thoughts @marcoesters? |
Agreed |
Ok, this can go in now that 3.10 is out. @marcoesters |
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 looks great!
I just have a couple of comments where I think we could use jinja a little more. If we use jinja's functions more, formatting and data are more cleanly separated and give developers a better idea about how the final product looks like without having to jump between py file and template.
|
||
data = data.replace("_SCRIPT_ENV_VARIABLES_=''", '\n'.join( | ||
[f"export {key}='{value}'" for key, value in custom_variables.items()])) | ||
variables["pkg_name_lower"] = pkg_name_lower |
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 can be done with jinja
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's a bit more involved... we need access to the info
dict in Jinja if we want to do that. See line 324.
|
||
# From now on, the items added to variables will NOT be escaped | ||
|
||
# These are mostly booleans we use with if-checks |
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.
Now that this is all one dictionary, this could be merged into the variables
initializer.
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.
We need to do it here separately because the block above have been stringified and escaped, but we want true booleans for these ones.
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.
LGTM - can you file an issue to improve Jinja integration so that we can put this on our radar?
There you go: #901, thanks! |
Description
It was about time.
Checklist - did you ...
news
directory (using the template) for the next release's release notes?