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

Add indent filter #151

Merged
merged 14 commits into from Nov 27, 2022
Merged

Add indent filter #151

merged 14 commits into from Nov 27, 2022

Conversation

Spiegie
Copy link
Contributor

@Spiegie Spiegie commented Nov 18, 2022

I missed a feature where i can ident a multiline Value. This PR should fix it.

Please be patient with me, this is my second PR and I'm new to Rust. I will do my best if something is missing.

@mitsuhiko
Copy link
Owner

The indent filter in Jinja2 does not indent the first line by default. Unclear if this is good behavior, but this means that the filters behave differently between the two engines. For me this is almost an argument to add this filter into an extension package with additional filters.

@Spiegie
Copy link
Contributor Author

Spiegie commented Nov 20, 2022

Thank you for your reply.
I will refactor the Code, considering your thoughts.

Maybe I could match the behavior of my Code to Jinja2s behavior (see https://jinja.palletsprojects.com/en/3.0.x/templates/#jinja-filters.indent). It seems like, they solved this, by adding some more arguments. I'll also remove the possibility of using tabs instead of spaces in favor of this. Unfortunately, rust is not good in keyword-based parameters (see rust-lang/rfcs#323), so the implementation could be very unintuitive. Would that path of action still be OK with you?

I'll also try to create an extension-package with filters, that don't try to math the behavior. There, I could create many separate indent-functions, depending on the current need. I hope, I can already dig into this, because it is very new to me. It should give us more flexibility, and I love this proposal. I will open a separate PR for this.

Should I create an Issue for that?

@mitsuhiko
Copy link
Owner

mitsuhiko commented Nov 20, 2022

For what it’s worth, MiniJinja has basic support for keyword arguments. If a filter is invoked with |foo(key=value) then an object is passed as additional argument. You can also detect this by looking at the is_kwargs attribute. There is however no fancy syntax support for this in the declaration of the parameter which might be something to look at. But in theory you can accept keyword arguments.

If you want send the PR against this here, and before merging I move it into a separate package.

@Spiegie
Copy link
Contributor Author

Spiegie commented Nov 20, 2022

Thank you for your support. I'll explore the keyword support. For now, I pushed my changed, so others can follow my progress. I change the PR title to WIP

@Spiegie Spiegie changed the title Add feature indent WIP: Add feature indent Nov 20, 2022
@Spiegie
Copy link
Contributor Author

Spiegie commented Nov 20, 2022

I also noticed, that my use of feature messed with my tests, and they were never executed. My code will need much more refactoring.

@Spiegie Spiegie changed the title WIP: Add feature indent Add feature indent Nov 27, 2022
@Spiegie
Copy link
Contributor Author

Spiegie commented Nov 27, 2022

I think, I can hand the code over for review.
I would suggest adding the filter as built in, because in jinja2 the filter is also built in, but you can of course separate it into a package.
I decided against indenting with a string feature, because this feature was added in jinja 3.0.

@mitsuhiko mitsuhiko changed the title Add feature indent Add indent filter Nov 27, 2022
@mitsuhiko mitsuhiko merged commit b0b18c3 into mitsuhiko:main Nov 27, 2022
@Spiegie Spiegie deleted the add-feature-indent branch November 28, 2022 21:02
@Spiegie
Copy link
Contributor Author

Spiegie commented Nov 28, 2022

Thank you for everything.

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.

None yet

2 participants