-
-
Notifications
You must be signed in to change notification settings - Fork 35
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
feat: Support for additional template variables #73
Conversation
Additional template variables for the Jinja template can be passed via CLI or can be defined in a configuration file. To pass variables via CLI, the option "--template-var KEY VALUE" can be given multiple times. To define variables in a configuration file, the variables can be defined in a "[template_vars]" table. [template_vars] KEY = "VALUE" All values are accessible in the template through the "template_vars" (dict) variable.
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.
Amazing feature once again! Thank you so much 😊
This all looks good, but I think I would prefer the following syntax on the CLI: --template-var key=value
. We can do key_value.split("=", 1)
so that values can have the =
character too. What do you think?
Also, I don't think I like "template var" for the name and variable/parameter names in the code. What do you think of "jinja context" instead? This way we can also add a short option with -j
which is not yet taken 🙂 I would then pass this context as context
or user_context
when rendering the template ("jinja" would be a bit redundant there).
Finally, it will need some documentation too! I can help with that if needed.
tests/test_cli.py
Outdated
k1 = "ignored" | ||
k2 = "v2" | ||
k3 = "v3" | ||
""".lstrip(), |
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.
lstrip
does nothing here, since the string starts with [
. You probably meant to use something like textwrap.dedent
, to dedent every line?
I first thought, that having key and value as separate parameters, would make it a bit easier, when calling it from a shell script (if the value has to be escaped). I am fine with
Naming is not something I am especially good at 😁, so whatever you prefer is fine for me.
I can add some documentation (though help on that would be appreciated). |
I've pushed some docs 😄 Feel free to tweak them. |
src/git_changelog/cli.py
Outdated
@@ -487,6 +521,7 @@ def build_and_render( | |||
bump: str | None = None, | |||
zerover: bool = True, # noqa: FBT001,FBT002 | |||
filter_commits: str | None = None, | |||
jinja_context: dict[str, str] | None = None, |
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 true that values passed from the CLI will always be strings, but when using the API directly from Python, values could be anything, so I think it should be dict[str, Any]
.
tests/test_cli.py
Outdated
|
||
assert contents == "k1 = v1\nk2 = v2\nk3 = v3\n" |
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.
assert contents == "k1 = v1\nk2 = v2\nk3 = v3\n" |
Awesome, thanks! I only, did a small change in the config file example. I changed it now to use KEY=VALUE and I hope, I addressed all other comments as well. |
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, thanks a lot 🙂
When using a custom Jinja template to render the changelog, it would be great, if additional context information could be accessed in that template. This feature request should be related to #17.
With the changes in this PR, additional context can be passed as key/value pairs either via CLI or via the toml config file.
To pass variables via CLI, the option
--template-var KEY VALUE
can be given multiple times.To define variables in a configuration file, the variables can be defined in a "[template_vars]" table.
When template vars are given via CLI and via config file, they are merged with the CLI vars taking precedence.
All values are accessible in the template through the "template_vars" (dict) variable.
The parsing of the CLI option into a dict is based on this gist: https://gist.github.com/vadimkantorov/37518ff88808af840884355c845049ea
(Unfortunately argparse does not support it out of the box, or at least I could not find a better way to achieve this)