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

Add urlescape to default filters #85

Merged
merged 2 commits into from Jan 7, 2012
Merged

Conversation

aaronsw
Copy link
Contributor

@aaronsw aaronsw commented Jan 6, 2012

It'd be great if urlescape was included in the list of default filters, since it's so commonly needed. Here's a patch that passes all the test cases.

@aaronsw
Copy link
Contributor Author

aaronsw commented Jan 6, 2012

If you like this, I can also add:

def datetimeformat(value, format='%d %B %Y %H:%M'):
    return value.strftime(format)

and

def skip(s, skip):
    return (k for k in s if k != skip)

Let me know if it's worth writing pull requests for them.

@mitsuhiko
Copy link
Contributor

For a long time I was against url encode filters since they are encoding specific but now that everybody agrees on utf-8 as URLs it's probably fine. However I would love to see the filter accept dictionaries as well and automatically encoding them correctly. Also one name for the filter is enough :-)

Lastly: needs tests.

@aaronsw
Copy link
Contributor Author

aaronsw commented Jan 7, 2012

It has tests! What's wrong with them?

@aaronsw
Copy link
Contributor Author

aaronsw commented Jan 7, 2012

What's wrong with having multiple names, especially when the function-name is so hard to remember like this one? It just makes it easier to use.

Nonetheless, I have removed them and implemented your other difficult requests. It feels a bit like a trial by fire. How's it look now?

@mitsuhiko mitsuhiko merged commit c299ff1 into pallets:master Jan 7, 2012
@mitsuhiko
Copy link
Contributor

It has tests! What's wrong with them?

Not sure how I missed them. Never mind then, but improved tests are always awesome :-)

What's wrong with having multiple names, especially when the function-name is so hard to remember like this one? It just makes it easier to use.

Because filters are intended to be sensible defaults to hook others in and especially with urlencode it would be awful if someone who wants a different encoding behavior would have to override all the aliases for it as well. Especially if new aliases are introduced later. We only use aliases for core functionality that you don't want to change or is changeable from a separate API hook (for instance the autoescaping mirrors to the |safe, |e and |escape filters).

It is encouraged to alias filters as necessary in your own application.

It feels a bit like a trial by fire.

I'd rather have my contributors go the extra mile for a better final result than to pull things I regret later. Learned that the hard way.

How's it look now?

I pulled it but had to rework the implementation for Python 3 compatibility.

@mitsuhiko
Copy link
Contributor

def datetimeformat(value, format='%d %B %Y %H:%M'):

In theory I'm all for that but the problem is that strftime on Python 2.x is not unicode safe. Also it does not work for dates before 1900. That's the reason why this was never added and we documented the use of babel instead.

def skip(s, skip):

What's the use case for this? Not that I'm against such a filter but it seems fairly specific.

@aaronsw
Copy link
Contributor Author

aaronsw commented Jan 7, 2012

Sorry about the Python 3 issues -- didn't think to test. And will try
to be better about style next time. Do you just run through pep8?

Your babel point makes sense and I'll submit skip if I find myself
using it a few more times.

@jace jace mentioned this pull request Feb 13, 2015
@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.

None yet

2 participants