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 striptags filter #589

Merged
merged 1 commit into from
Nov 19, 2015
Merged

Add striptags filter #589

merged 1 commit into from
Nov 19, 2015

Conversation

antogyn
Copy link
Contributor

@antogyn antogyn commented Nov 18, 2015

I migrated from swig to nunjucks and realized there was no striptags filter. There is already an opened issue : #359

One of the tests is very similar to this one (from jinja)

@carljm
Copy link
Contributor

carljm commented Nov 18, 2015

Looks like the build is failing. I'd be happy to merge this with a green build.

@antogyn
Copy link
Contributor Author

antogyn commented Nov 18, 2015

The build is also failing on master (of this repo) with the same messages on my local machine with node 0.10 and 0.11. I don't think it comes from this PR

@antogyn
Copy link
Contributor Author

antogyn commented Nov 18, 2015

There seems to be a problem with the last version of jshint. Using jshint 2.8.x fixes the problem on both versions. Should I make a second PR to fix this ?

@carljm
Copy link
Contributor

carljm commented Nov 18, 2015

Just pushed a fix to master to pin jshint to 2.8.x. Rebase on that and see if we get a green build.

@antogyn
Copy link
Contributor Author

antogyn commented Nov 19, 2015

Done.

carljm added a commit that referenced this pull request Nov 19, 2015
@carljm carljm merged commit b7bdd7a into mozilla:master Nov 19, 2015
@ivan-kleshnin
Copy link
Contributor

Not a very good implementation because it strips whitespace.
You often need to pipe striptags, nl2br, safe. For example when you're rendering email template from feedback form. You want to remove tags keeping original linebreaks.

{{ text | striptags | nl2br | safe }}

But nl2br is useless here because striptags just erases all whitespace.

.replace(/\s+/gi, ' ')

It would be better to squash every group of multiple spaces to single space and every group of multiple caret feeds to single one separately. Something like:

.replace(/ +/g, ' ').replace(/(\r\n?)+/g, '\n').replace(/\n+/g, '\n')

@carljm
Copy link
Contributor

carljm commented Dec 5, 2015

Can you file a pull request?

@ivan-kleshnin
Copy link
Contributor

Yes, I'll give it a second look and make a PR.

@carljm
Copy link
Contributor

carljm commented Dec 5, 2015

Thank you!

@antogyn
Copy link
Contributor Author

antogyn commented Dec 8, 2015

Keep in mind that what you propose is different from jinja's implementation and won't pass the same test jinja has.

@ivan-kleshnin
Copy link
Contributor

Yes, I'll update tests accordingly.

@carljm
Copy link
Contributor

carljm commented Dec 8, 2015

Hmmm. Thanks for pointing that out, @antogyn. I think I prefer to maintain Jinja2 compatibility here by default, but I'm not opposed to adding this as an optional behavior controlled by a flag, e.g. preserve_whitespace=true (defaulting to false).

@ivan-kleshnin
Copy link
Contributor

Yes, I was going to put it under non-default flag. Will try to send PR tomorrow.

@ivan-kleshnin
Copy link
Contributor

Sorry for delay. A tons of work activity.
Recently sent this: #619
I named it preserve_linebreaks because it rather normalizes whitespace (and linebreaks...).

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