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

Adding Jinja2TemplateFilter #20

Merged
merged 3 commits into from Jul 14, 2012
Merged

Adding Jinja2TemplateFilter #20

merged 3 commits into from Jul 14, 2012

Conversation

rdegges
Copy link
Contributor

@rdegges rdegges commented Jul 13, 2012

Heya, this references issue #19. I thought I'd submit this pull request, which adds a Jinja2TemplateFilter to flask-assets (exposed under the name jinja2template). It is automatically registered when flask_assets is imported, so it should be usable out of the box.

This obviously requires the latest development release of webassets to run (since I just got my initial jinja2 filter accepted there), so I'm not sure if you'd like to merge this now, or wait till webassets makes a new stable release.

Either way, I thought I'd leave this here to see what you think.

This allows us to run static assets through Jinja2's templateing engine (using
Flask's standard template contexts).
…ets`.

This was, the temalte filter will be available to any flask-assets users.
from webassets.filter import Filter, register_filter


class Jinja2TemplateFilter(Filter):
Copy link
Owner

Choose a reason for hiding this comment

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

I think we should keep the class names in line with webassets, i.e. without a suffix.

@miracle2k
Copy link
Owner

Does the packaging work with this separate file? It will be configured to install flask_assets.py as a root-level module, and probably skip filter.py completely.

You will need to make flask_assets folder, and put filter.py inside, and rename flask_assets.py to flask_assets/__init__.py.

Alternatively, while splitting the code across multiple files will likely have to happen sooner or later, it would be easiest for now to just define the filter class inside flask_assets.py (in which case I would keep the Filter suffix in the class name).

@miracle2k
Copy link
Owner

Also, and I am a bit on the fence on this, jinja2template is a bit unyieldy, given that Flask users almost certainly will prefer to use it instead of the original.

How do you feel about making it replace the base jinja2 filter in the name registry? If the user really wants to use the original filter, and I don't really see why he would, given that there's a way to use a custom context in the Flask version, it's always possible to create an instance of webassets.filter.jinja2 directly.

@rdegges
Copy link
Contributor Author

rdegges commented Jul 13, 2012

That's a great point, re: overriding the base jinja2 filter in the registry. I didn't really consider that, but it makes perfect sense. I largely based this patch on the django-assets custom template filter, and didn't really think about that at all.

Also: from my testing, the filter.py fille works fine separately. The way I tested it so far was just to run the entire thing via setup.py develop, so that may not have been the best test.

Why don't I just move the filter inside the flask_assets.py file for now (to avoid complications), and leave the class name as-is (with the Template suffix). Whenever you want to refactor, we can break it out, and I can help get things working (if that's alright with you).

So, for now I'll:

  • Move the filter into the flask_assets.py file,
  • Have it override the jinja2 webassets filter instead of registering a new filter.

Is that acceptable?

@miracle2k
Copy link
Owner

Sounds great.

FWIW, what presumably happens is that it installs filter.py globally, which wouldn't be right.

@rdegges
Copy link
Contributor Author

rdegges commented Jul 14, 2012

Hey, so I'm working on this right now, but I've actually got a question. In webassets, the register_filter function: https://github.com/miracle2k/webassets/blob/master/src/webassets/filter/__init__.py#L532-L541 won't allow me to override a filter that has already been defined.

So that I don't break any style conventions here, would you prefer that I:

  • Remove the duplicate check, and allow register_filter to overwrite an already registered filter? or
  • Manually import _FILTERS from webassets and remove the original jinja2 filter before adding the Flask specific one?

@miracle2k
Copy link
Owner

I had assumed that register_filter() would allow overwriting; so that's what I would suggest to do. It makes sense that there would be an "official" way to accomplish this.

And having it override the build-in ``jinja2`` template, so that we get the
included Flask contexts by default.
@rdegges
Copy link
Contributor Author

rdegges commented Jul 14, 2012

So, just updated this pull request like we discussed. It works locally (when tested). Obviously, this will still not work unless webassets get a new pypi release, and this gets a forced webassets upgrade via setup.py, since this relies on the latest webassets merges :)

@miracle2k miracle2k merged commit 3e3477f into miracle2k:master Jul 14, 2012
@miracle2k
Copy link
Owner

Actually, installing the development version should automatically pull the dev version of webassets too :)

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

3 participants