-
Notifications
You must be signed in to change notification settings - Fork 85
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
Cleanup config files #141
Cleanup config files #141
Conversation
@n-rodriguez I've done a brief eyeball check and this looks like a significant and excellent contribution. The only comment I have is regarding the use of Jinja whitespace control, specifically doubling up the
Basically, as a rough guideline to minimise formula breakages, limit whitespace control to only a single |
Yes, I understand this kind of concern, but actually the '-' I added are here to remove empty lines at the top of files. The goal was to trim whitespaces so I was very careful to not change the logic implemented. |
@n-rodriguez I fully understand. Have you tried setting the following in your # Old version:
# jinja_trim_blocks: True
# New version:
jinja_env:
trim_blocks: True I was amazed at how aggressive this is! I spent a fair while with this when arguing things in the upstream issue and I found it broke (and I mean broke) so many formulas. I've asked for this to be improved, where we can override this setting as formula writers, but until then we're going to have to be cautious about "excessive" whitespace control. Ultimately, it's your PR and this is only a general suggestion, nothing enforced. |
Fixed! |
@n-rodriguez Just the last ones that slipped through... after these, I'll leave you alone, promise! |
done |
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.
@n-rodriguez Thanks for going to a lot of effort to clean up these files. From my part, I'm satisfied that this is a worthy PR. I'm approving this based on my eyeball review. Due to my lack of familiarity with this formula itself, I'll leave it to the others to dissect the actual impact of the changes.
This PR remove white spaces in plugins config files.