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

jinja2 offline compression bug #981

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

nwilson5
Copy link
Contributor

Re: #973
Jinja2 added _elif, compress inside an elif tag getting missed in offline compression.

@nwilson5
Copy link
Contributor Author

Not super familiar with writing tests, feel free to change it as you see fit.

@karyon
Copy link
Contributor

karyon commented Dec 24, 2019

Thanks! how did you run the tests? specifically, it's rather important that you have requirements/tests.txt installed and use compressor/test_settings.py as settings. also, please verify that the output files (which are pointed at by the new script tags) contain what you expect.

also, your PR contains some old commits. That doesn't matter much though, we can just squash-merge the PR so those will be gone.

@nwilson5
Copy link
Contributor Author

Thanks! how did you run the tests? specifically, it's rather important that you have requirements/tests.txt installed and use compressor/test_settings.py as settings. also, please verify that the output files (which are pointed at by the new script tags) contain what you expect.

also, your PR contains some old commits. That doesn't matter much though, we can just squash-merge the PR so those will be gone.

I'm unfamiliar with writing tests or contributing to django-compressor specifically, so not sure what actually needs to be done here. I am not interested in figuring out how to create a viable pull request to fix this issue. I've forked and fixed this for my needs. It is a bug that needs to be fixed, and it's a fairly simple fix, so if the contributors to this package feel it worthwhile you're welcome to sort out the tests.

@karyon
Copy link
Contributor

karyon commented May 10, 2020

for the protocol, the change seems correct, but the newly added tests are failing. if anyone wanto to pick this up, you'd need to debug the test and verify that the rendered output of _render_script and _render_result in _test_offline look good, and then adjust the hashes.

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

4 participants