Improvement on filters #66

Closed
wants to merge 9 commits into
from

Conversation

3 participants
@snoack
Contributor

snoack commented Oct 10, 2011

I have added the template filters, listed below and have done some improvements on existing filters.

  • min
  • max
  • filter
  • filterfalse
  • map
  • unique

You find further details including examples in the docstring of those filter functions.

@mitsuhiko

This comment has been minimized.

Show comment Hide comment
@mitsuhiko

mitsuhiko Oct 10, 2011

Owner

The filter thing is pretty neat. What concerns me a bit is that it does not support arguments for the tests which might be a problem. For instance divisibleby is not useable with filter. Maybe have a filtertest() that forwards all arguments to the test function. Generally however that looks pretty cool and I would be happy pulling something like that.

I will play a bit around with how it works and how it could be improved.

Owner

mitsuhiko commented Oct 10, 2011

The filter thing is pretty neat. What concerns me a bit is that it does not support arguments for the tests which might be a problem. For instance divisibleby is not useable with filter. Maybe have a filtertest() that forwards all arguments to the test function. Generally however that looks pretty cool and I would be happy pulling something like that.

I will play a bit around with how it works and how it could be improved.

@mitsuhiko

This comment has been minimized.

Show comment Hide comment
@mitsuhiko

mitsuhiko Oct 10, 2011

Owner

Another thing is that i want to slowly deprecated 2.4 so that hack won't be necessary for 2.4. If 2.4 is detected just raise a runtime error if the key parameter is used.

Owner

mitsuhiko commented Oct 10, 2011

Another thing is that i want to slowly deprecated 2.4 so that hack won't be necessary for 2.4. If 2.4 is detected just raise a runtime error if the key parameter is used.

@snoack

This comment has been minimized.

Show comment Hide comment
@snoack

snoack Oct 10, 2011

Contributor

I have updated my better-filters branch. Now tests, taking arguments are supported. All positional arguments are passed to the test, while attribute and invert are required to be keyword arguments if given. I am quiet satisfied with that implementation. What do you think?

Contributor

snoack commented Oct 10, 2011

I have updated my better-filters branch. Now tests, taking arguments are supported. All positional arguments are passed to the test, while attribute and invert are required to be keyword arguments if given. I am quiet satisfied with that implementation. What do you think?

@snoack

This comment has been minimized.

Show comment Hide comment
@snoack

snoack Oct 11, 2011

Contributor

By the way, I noticed an inconsistency on filters expecting iterables. Most filters leave the TypeError raised by the underlying built-in function (iter, reversed, list, sum, etc.), if the given value isn't an iterable, unhandled. But the reverse filter catches that TypeError and raises his own FilterArgumentError instead.

In my opinion, all filters should deal with iterables in the same way. So in favor of less and simpler code, I would suggest to always let the underlying TypeError raise to the top instead of putting it into a FilterArgumentError, in the case of the reverse filter. Would you agree on that?

Contributor

snoack commented Oct 11, 2011

By the way, I noticed an inconsistency on filters expecting iterables. Most filters leave the TypeError raised by the underlying built-in function (iter, reversed, list, sum, etc.), if the given value isn't an iterable, unhandled. But the reverse filter catches that TypeError and raises his own FilterArgumentError instead.

In my opinion, all filters should deal with iterables in the same way. So in favor of less and simpler code, I would suggest to always let the underlying TypeError raise to the top instead of putting it into a FilterArgumentError, in the case of the reverse filter. Would you agree on that?

@snoack

This comment has been minimized.

Show comment Hide comment
@snoack

snoack Oct 11, 2011

Contributor

I have got another question. Is it a bug or the intended behavior that the groupby filter does not normalize the case of strings as the sort and dictsort filter does?

That way it seems some kind of inconsistent to me. But on the other hand if we would just do make_sort_func(make_attrgetter(...), case_sensitive) for the groupby filter, it would convert each returned grouper to lowercase (if case_sensitive=False), which might confuse the template authors.

Contributor

snoack commented Oct 11, 2011

I have got another question. Is it a bug or the intended behavior that the groupby filter does not normalize the case of strings as the sort and dictsort filter does?

That way it seems some kind of inconsistent to me. But on the other hand if we would just do make_sort_func(make_attrgetter(...), case_sensitive) for the groupby filter, it would convert each returned grouper to lowercase (if case_sensitive=False), which might confuse the template authors.

@snoack

This comment has been minimized.

Show comment Hide comment
@snoack

snoack Oct 11, 2011

Contributor

My latest commit made the groupby filter case insensitive by default, just like the sort and dictsort filter, but does not convert the returned groupers to lowercase. That fixes the inconsistency without confusing template authors or breaking existing code (too much). What do you think?

Contributor

snoack commented Oct 11, 2011

My latest commit made the groupby filter case insensitive by default, just like the sort and dictsort filter, but does not convert the returned groupers to lowercase. That fixes the inconsistency without confusing template authors or breaking existing code (too much). What do you think?

@mitsuhiko

This comment has been minimized.

Show comment Hide comment
@mitsuhiko

mitsuhiko Oct 11, 2011

Owner

Generally that looks all pretty good. For the filter i would also forward the kwargs to the test and only pop attribute if test is not passed and prefix invert with an underscore:

{{ foo|filter(test='my_test') }}
{{ foo|filter(test='my_test', these, are='args') }}
{{ foo|filter(attribute='my_test', _invert=true) }}

The reason for prefixing _invert would be that this way clashes with a test having an keyword argument named _invert is less likely.

The groupby change I think makes sense. I think it should not cause any problems in practice.

Regarding TypeError vs FilterArgumentError I think it would make sense to have a helper that asserts the type and otherwise raises the FilterArgumentError. Reasoning for that: if filters raise a FitlerArgumentError you can catch that down easily and do something else. This makes it possible to better try something and if it fails, attempt something else instead without hiding errors.

Owner

mitsuhiko commented Oct 11, 2011

Generally that looks all pretty good. For the filter i would also forward the kwargs to the test and only pop attribute if test is not passed and prefix invert with an underscore:

{{ foo|filter(test='my_test') }}
{{ foo|filter(test='my_test', these, are='args') }}
{{ foo|filter(attribute='my_test', _invert=true) }}

The reason for prefixing _invert would be that this way clashes with a test having an keyword argument named _invert is less likely.

The groupby change I think makes sense. I think it should not cause any problems in practice.

Regarding TypeError vs FilterArgumentError I think it would make sense to have a helper that asserts the type and otherwise raises the FilterArgumentError. Reasoning for that: if filters raise a FitlerArgumentError you can catch that down easily and do something else. This makes it possible to better try something and if it fails, attempt something else instead without hiding errors.

@mitsuhiko

This comment has been minimized.

Show comment Hide comment
@mitsuhiko

mitsuhiko Oct 11, 2011

Owner

Something else that I think would be cool to have is a map function that works like filter but does what python's map does and uses filters instead of tests:

{% for username in users|map(attribute='username') %}
  {{ username }}
{% endfor %}

{% for username in users|map(attribute='username')|map(filter='upper') %}
  {{ username }}
{% endfor %}

etc.

Owner

mitsuhiko commented Oct 11, 2011

Something else that I think would be cool to have is a map function that works like filter but does what python's map does and uses filters instead of tests:

{% for username in users|map(attribute='username') %}
  {{ username }}
{% endfor %}

{% for username in users|map(attribute='username')|map(filter='upper') %}
  {{ username }}
{% endfor %}

etc.

@snoack

This comment has been minimized.

Show comment Hide comment
@snoack

snoack Oct 12, 2011

Contributor

I don't like the idea of adding an underscore as prefix to the invert keyword argument of the filter filter, because of prefixed arguments look ugly and an underscore as prefix often means that it is an internal argument that should not be used. Also passing the attribute argument to the test if given, but giving an other meaning to that argument if no test was given, might confuse template authors and feels inconsistent to me. I think passing positional arguments to the test, while interpreting keyword arguments by the filter itself is straightforward and easy to understand for template authors. Most built-in tests don't take additional arguments, anyway.

How do you mean that a FilterArgumentError can be catched in order to try something else, in a way that you can't do with a TypeError? However preemptive type or constraint checks are anything but pythonic. Have a look at the reversed and last filter for example. If the object processed by those filters, implements __reversed__ it doesn't matter whether the object itself is iterable, but instead the object returned by __reversed__ must be an iterator. So you see that the correct type/constraint check strongly depends on what exactly the filter does and can become very complex. Also that check is redundant, because of in the case it fails, a TypeError is raised anyway. I don't care much whether a FilterArgumentError or TypeError is raised in that case, as far as it is consistent among all filters. So what do you think?

I already thought, about adding a map filter that works exactly the same. But when adding the map filter I would like to deprecate the attribute argument for the join and sum filter. In my opinion there is no reason that each filter that takes an iterable, must support attribute lookups anymore, when you can just use the map filter like below:

{{ foo|map(attribute='bar')|join(',') }}
{{ foo|map(attribute='bar')|sum }}

Do you agree on that and if so how should we deprecate it? Of course we could just call warnings.warn(..., DeprecationWarning), if the attribute argument is given. But dependant on how people use jinja2 in their applications that warning might stay invisible to template authors. But if you don't have a better idea, that might be still reasonable.

Contributor

snoack commented Oct 12, 2011

I don't like the idea of adding an underscore as prefix to the invert keyword argument of the filter filter, because of prefixed arguments look ugly and an underscore as prefix often means that it is an internal argument that should not be used. Also passing the attribute argument to the test if given, but giving an other meaning to that argument if no test was given, might confuse template authors and feels inconsistent to me. I think passing positional arguments to the test, while interpreting keyword arguments by the filter itself is straightforward and easy to understand for template authors. Most built-in tests don't take additional arguments, anyway.

How do you mean that a FilterArgumentError can be catched in order to try something else, in a way that you can't do with a TypeError? However preemptive type or constraint checks are anything but pythonic. Have a look at the reversed and last filter for example. If the object processed by those filters, implements __reversed__ it doesn't matter whether the object itself is iterable, but instead the object returned by __reversed__ must be an iterator. So you see that the correct type/constraint check strongly depends on what exactly the filter does and can become very complex. Also that check is redundant, because of in the case it fails, a TypeError is raised anyway. I don't care much whether a FilterArgumentError or TypeError is raised in that case, as far as it is consistent among all filters. So what do you think?

I already thought, about adding a map filter that works exactly the same. But when adding the map filter I would like to deprecate the attribute argument for the join and sum filter. In my opinion there is no reason that each filter that takes an iterable, must support attribute lookups anymore, when you can just use the map filter like below:

{{ foo|map(attribute='bar')|join(',') }}
{{ foo|map(attribute='bar')|sum }}

Do you agree on that and if so how should we deprecate it? Of course we could just call warnings.warn(..., DeprecationWarning), if the attribute argument is given. But dependant on how people use jinja2 in their applications that warning might stay invisible to template authors. But if you don't have a better idea, that might be still reasonable.

@mitsuhiko

This comment has been minimized.

Show comment Hide comment
@mitsuhiko

mitsuhiko Oct 12, 2011

Owner

I don't like deprecating things in the template language aspect of Jinja2. There is not much value in that. Also the way that join and sum work is that they don't have to buffer up things if they take the attribute directly and it's more expressive. I don't mind having more than one way to do things in the template aspect of things if this means that I don't break backwards compat.

Regarding the FitlerArgumentError the usecase is that if you know that an invalid argument to the filter raises that exception instead of type error you can catch down an invalid invocation separately from something inside the filter breaking because of a bug in the filter. Especially in combination with the sandboxed execution that is a nice feature to have.

Owner

mitsuhiko commented Oct 12, 2011

I don't like deprecating things in the template language aspect of Jinja2. There is not much value in that. Also the way that join and sum work is that they don't have to buffer up things if they take the attribute directly and it's more expressive. I don't mind having more than one way to do things in the template aspect of things if this means that I don't break backwards compat.

Regarding the FitlerArgumentError the usecase is that if you know that an invalid argument to the filter raises that exception instead of type error you can catch down an invalid invocation separately from something inside the filter breaking because of a bug in the filter. Especially in combination with the sandboxed execution that is a nice feature to have.

@mitsuhiko

This comment has been minimized.

Show comment Hide comment
@mitsuhiko

mitsuhiko Oct 12, 2011

Owner

Instead of the invert I would add a filterfalse in that case.

Owner

mitsuhiko commented Oct 12, 2011

Instead of the invert I would add a filterfalse in that case.

@snoack snoack closed this Oct 27, 2011

@snoack snoack reopened this Oct 27, 2011

@snoack

This comment has been minimized.

Show comment Hide comment
@snoack

snoack Oct 27, 2011

Contributor

Sorry, for accidentally closing the pull request.

You might also have a look at Pull Request #57. It suggests using getattr() instead of getitem() in make_attrgetter(), which absolutely makes sense, in my opinion. There is also Pull Request #42, which I recommend to reject in favor of the filter filter.

Contributor

snoack commented Oct 27, 2011

Sorry, for accidentally closing the pull request.

You might also have a look at Pull Request #57. It suggests using getattr() instead of getitem() in make_attrgetter(), which absolutely makes sense, in my opinion. There is also Pull Request #42, which I recommend to reject in favor of the filter filter.

snoack added some commits Oct 7, 2011

Refactored 'filter' filter and added 'map' filter.
* Keyword arguments are passed in addition to positional arguments
  to the test function, now.
* The 'invert' keyword argument was removed in favor of a new
  filter called 'filterfalse'.
* The new filter 'map' was added. It shares most of its code
  with the 'filter' and 'filterfalse' filters, but expects
  a filter instead of test and calls map() instead of filter().
Added 'strict' keyword argument to Environment.getitem and Environmen…
…t.getattr.

That way we don't need to repeat the logic in the `attr` filter.
Also it simplifies the implementation of the derived SandboxedEnvironment.

mitsuhiko added a commit that referenced this pull request May 19, 2013

@mitsuhiko

This comment has been minimized.

Show comment Hide comment
@mitsuhiko

mitsuhiko May 19, 2013

Owner

A small version of this is now implemented in a different form.

Owner

mitsuhiko commented May 19, 2013

A small version of this is now implemented in a different form.

@mitsuhiko mitsuhiko closed this May 19, 2013

@renierdbruyn

This comment has been minimized.

Show comment Hide comment
@renierdbruyn

renierdbruyn Jan 17, 2014

Sorry for posting on an old issue, but why did you not add the min and max filters? I am in need of such a filter... How can I do this without the filters?

Sorry for posting on an old issue, but why did you not add the min and max filters? I am in need of such a filter... How can I do this without the filters?

@snoack

This comment has been minimized.

Show comment Hide comment
@snoack

snoack Aug 7, 2015

Contributor

I have submitted new pull requests for adding the unique filter (#469) and the min/max filters (#475).

Contributor

snoack commented Aug 7, 2015

I have submitted new pull requests for adding the unique filter (#469) and the min/max filters (#475).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment