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

Added tests for Jinja2. #61

Merged
merged 2 commits into from
May 7, 2017
Merged

Added tests for Jinja2. #61

merged 2 commits into from
May 7, 2017

Conversation

AndreasBackx
Copy link
Collaborator

@AndreasBackx AndreasBackx commented May 7, 2017

This is a follow-up on #58. Tests have been added for Jinja2. Following is a checklist of what is or has yet to be done before this PR can be merged.

@AndreasBackx AndreasBackx self-assigned this May 7, 2017
* Resolves jrief#49.
* Fixes failing Python 2.7 Jinja2 tests.
* Multiple engines are now supported in the compilescss command.
* Added more info to README regarding Jinja2 compilescss support.
@AndreasBackx
Copy link
Collaborator Author

Note my commit message on the latest commit:

Added tests for compilescss management command.

* Resolves #49.
* Fixes failing Python 2.7 Jinja2 tests.
* Multiple engines are now supported in the compilescss command.
* Added more info to README regarding Jinja2 compilescss support.

I've added support for using multiple engines in compilescss as well:

manage.py compilescss --engine django --engine jinja2

I stumbled upon BaseCommand.requires_system_checks that we could set to False so the command can still without needing to run the system checks. Maybe we could look into this? django-compressor uses it as well.

Shouldn't we add the license from django-compressor because we use parts of their management command's code? They also do it because they have some code from django-compress, see their LICENSE file. While we're at it, can we rename the LICENSE-MIT file to LICENSE?

@AndreasBackx AndreasBackx requested a review from jrief May 7, 2017 14:35
@@ -49,7 +50,7 @@ def __init__(self):
self.parser = None
self.template_exts = getattr(settings, 'SASS_TEMPLATE_EXTS', ['.html'])
self.sass_output_style = getattr(settings, 'SASS_OUTPUT_STYLE',
'nested' if settings.DEBUG else 'compressed')
'nested' if settings.DEBUG else 'compressed')
Copy link
Collaborator Author

@AndreasBackx AndreasBackx May 7, 2017

Choose a reason for hiding this comment

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

My code formatter did this, I can fix this in a follow-up commit along with the other odd ones to make them look less odd after a review. Perhaps to something along these lines:

self.sass_output_style = getattr(
    settings,
    'SASS_OUTPUT_STYLE',
    'nested' if settings.DEBUG else 'compressed'
)

@@ -2,31 +2,32 @@
from __future__ import unicode_literals

import os

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I apply isort to all of my files, maybe we should start doing this everywhere and add a check for it in Travis along with flake8?

Copy link
Owner

Choose a reason for hiding this comment

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

I'll check another project and use its isort config

@@ -12,6 +12,7 @@ deps =
pytest==3.0.7
pytest-django==3.1.2
django-compressor==2.1.1
jinja2==2.9.6
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Currently it isn't very intuitive to test multiple versions locally. Could we revamp the tox.ini to make this easier? I wrote one for django-safedelete as well.

@jrief jrief merged commit 1b93576 into jrief:master May 7, 2017
@jrief
Copy link
Owner

jrief commented May 7, 2017

Thank you so much!

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.

compilescss does not support jinja2
2 participants