Skip to content

Fix unintended lstrip_blocks behavior #1183

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

Merged

Conversation

petee-d
Copy link

@petee-d petee-d commented Apr 2, 2020

Introduced in #858. Closes #1138

@petee-d petee-d changed the title WIP: Fix unintended lstrip_blocks behavior. Fixes #1138 Fix unintended lstrip_blocks behavior. Fixes #1138 Apr 3, 2020
@petee-d
Copy link
Author

petee-d commented Apr 3, 2020

I ran a comparison on the resulting parse tree on 5GBs worth of over 100k Jinja templates created by users with various technical backgrounds and saw no difference. So apparently none of these relied on the behavior that this fixes, but it also shows it's unlikely this produces another undesired change of behavior.

I also performance tested this and there is no measurable change in performance. Also including the performance before the first fix that introduced this change.
image
X axis is log-rounded seconds, Y axis is the count of templates in the bucket.

@davidism davidism added this to the 2.11.2 milestone Apr 3, 2020
@petee-d
Copy link
Author

petee-d commented Apr 8, 2020

Today I got back to this for a bit and realized that the way I was testing it on my 100k sample templates was wrong, I didn't actually enable lstrip_blocks in my environment. So I repeated the test and the result is great. The generated parse trees after #857 were actually different in many cases from the parse trees generated before #857, and the parse trees after #1183 were in all cases the same as the ones before #857. I made the same mistake when comparing the parse trees when implementing #857 so this shows how the change of behavior went unnoticed and it also confirms that this fix returns to the correct behavior while keeping the much better performance.

@davidism
Copy link
Member

davidism commented Apr 8, 2020

Awesome work, thanks for following up. I've actually got this checked out right now, rebasing to 2.11.x and adding a changelog. Should be released later today.

@davidism
Copy link
Member

davidism commented Apr 8, 2020

I'm not sure how it happened, but you opened a PR from a different user (xponea), so I can't force push to the branch. If there's an option to allow pushes on the PR, please enable it. Otherwise, can you do the following:

git rebase --onto origin/2.11.x origin/master
nano CHANGES.rst  # add changelog entry to bottom of 2.11.2 section, given below
git commit -a --amend --no-edit
git push -f

Changelog entry:

-   Fix whitespace being removed before tags in the middle of lines when
    ``lstrip_blocks`` is enabled. :issue:`1138`

@davidism davidism changed the title Fix unintended lstrip_blocks behavior. Fixes #1138 Fix unintended lstrip_blocks behavior Apr 8, 2020
@davidism
Copy link
Member

davidism commented Apr 9, 2020

@petee-d ping on getting this rebased

Peter Dolak added 2 commits April 13, 2020 14:13
Introduced in pallets#858. Tests will follow, also results of performance
testing.
Also did peformance tests for the previous fix and saw no measurable
impact.
@petee-d petee-d force-pushed the 1138-fix-unintended-lstrip-behavior-change branch from dc6445b to 7163513 Compare April 13, 2020 12:14
@petee-d
Copy link
Author

petee-d commented Apr 13, 2020

@davidism Sorry for the delay, I was mostly offline the past few days. Pushed the rebase and change log entry to my fork branch. I don't see the option to let you make edits to this PR created from a fork, as described in https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/allowing-changes-to-a-pull-request-branch-created-from-a-fork. If you need to do more edits, feel free to copy the branch from my fork and recreate the PR.

@davidism davidism changed the base branch from master to 2.11.x April 13, 2020 13:54
@davidism davidism merged commit f1756a3 into pallets:2.11.x Apr 13, 2020
@davidism
Copy link
Member

Looks good, thanks!

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

lstrip_blocks doesn't preserve space before end block on the same line
2 participants