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

Enable all features by default in minijinja-cli #633

Merged
merged 4 commits into from
Nov 4, 2024

Conversation

KonishchevDmitry
Copy link
Contributor

The doc says that all features are enabled by default, but it's not true: by default (and in the release binaries too) ini format support and shell completion aren't enabled + ini format is actually partially broken even when enabled (can't be specified in the command line).

This PR fixes the issue.

@KonishchevDmitry
Copy link
Contributor Author

KonishchevDmitry commented Nov 4, 2024

@mitsuhiko, I've actually faced with yet another problem with ini format. I have a *.conf file which I use in both shell scripts and for template generation. And in this file all variables are uppercase (as it often used in *.env files). But minijinja-cli loads them in lowercase, which was unexpected for me.

Is it intentional? Personally, I would prefer that my variables are passed as-is to the templates. It can be fixed by using configparser::ini::Ini::new_cs() instead of configparser::ini::Ini::new(). I haven't done this in this PR, but can add the change if you don't mind. At least it would be consistent with --env option, which passes environment variables as-is.

@KonishchevDmitry KonishchevDmitry changed the title Enable all features by default Enable all features by default in minijinja-cli Nov 4, 2024
@mitsuhiko
Copy link
Owner

Yes, this should be fixed. Definitely unexpected and unintended. Could you also add a test for that?

@mitsuhiko mitsuhiko merged commit 009ff28 into mitsuhiko:main Nov 4, 2024
20 checks passed
@KonishchevDmitry KonishchevDmitry deleted the enable-ini branch November 5, 2024 04:18
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.

2 participants