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 Jinja whitespace control #224

Conversation

joaopgrassi
Copy link
Member

Closes #220

I added the new properties to the existing struct TemplateSyntax but I'm not so convinced that's the best place. I can see how it makes sense as the "Jinja config things" but 🤔. I'm open to suggestions. Maybe a different struct to configure jinja environment "in general"? Or maybe rename TemplateSyntax to something else, so it not only contain syntax things but general Jinja settings?

@joaopgrassi
Copy link
Member Author

weird sometimes the test fails also on my machine for the line end of the file. I will look into it

Copy link
Contributor

@lquerel lquerel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of integrating it into 'TemplateSyntax', I propose creating a separate structure in parallel that we could name 'WhitespaceControl', containing the two fields you created. Otherwise your PR LGTM. Thanks!

@joaopgrassi joaopgrassi requested a review from lquerel June 28, 2024 08:27
Copy link
Contributor

@lquerel lquerel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few minor modifications to make, but otherwise, it seems ready to be merged.

crates/weaver_forge/src/config.rs Outdated Show resolved Hide resolved
crates/weaver_forge/src/config.rs Outdated Show resolved Hide resolved
crates/weaver_forge/src/config.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@jsuereth jsuereth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I echo @lquerel's comments on default trait. Once that's cleaned up we can merge.

@joaopgrassi
Copy link
Member Author

If I got it right, I don't need the defaults annotations and functions, and I also removed the constructor. Sorry still very noob in Rust :D. Hope it is alright now.

@joaopgrassi
Copy link
Member Author

Not sure why the build is stuck? 🤔. I don't have permission to retry it.

@trisch-me
Copy link
Contributor

it's not stuck, I think it's because your test has failed

@lquerel lquerel merged commit 502c5f3 into open-telemetry:main Jul 2, 2024
22 checks passed
@joaopgrassi joaopgrassi deleted the feat/jinja-whitespace-control branch July 2, 2024 12:19
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.

Add whitespace control in Jinja templates
4 participants