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

Implement lstripBlocks and trimBlocks from Jinja2 #355

Merged
merged 2 commits into from Feb 4, 2015

Conversation

radev
Copy link
Contributor

@radev radev commented Jan 17, 2015

This pull request implements functionality from Jinja2 trim_blocks and lstrip_blocks (renaming to camelCase).
It can significantly reduce amount of whitespace in the output if a lot of block tags are used.

@garygreen
Copy link
Contributor

Thanks for this. Only briefly looked at the code, does this handle windows-style line breaks too \r\n?

@@ -1103,7 +1103,7 @@ var Compiler = Object.extend({
// console.log(tmpl);

module.exports = {
compile: function(src, asyncFilters, extensions, name, lexerTags) {
compile: function(src, asyncFilters, extensions, name, lexerTags, trimBlocks, lstripBlocks) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should rethink this, the number of parameters being passed here is a lot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can pass env instead of lexerTags, trimBlocks, lstripBlocks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, should I pass environment to lexer?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's pass around a options object instead. If you can, move all the arguments after src into an options argument and us it like opts.trimBlocks. I think it would be fine to pass this same options argument to the parser and lexer as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Problem is that path is not from opts and it's fourth argument. So either asyncFilters and extensions should be left where they are now. Or we break backwards compatibility.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't care about breaking backwards compatibility as this will be going in nunjucks 2.0

@radev
Copy link
Contributor Author

radev commented Jan 17, 2015

No, this doesn't support windows-style line breaks, but Jinja2 doesn't support them too:
https://github.com/mitsuhiko/jinja2/blob/c97fa109d38b5367ce3efffb6e435d007dc55920/jinja2/lexer.py#L434

@jlongster
Copy link
Contributor

Thanks for doing this, it looks good.

@radev
Copy link
Contributor Author

radev commented Jan 24, 2015

I've moved some Environment options to opts, but left filters, asyncFilters, extensions as is — they are mutable by Environment.

@jlongster
Copy link
Contributor

Thanks, I like how this looks. I'd say we go ahead and merge it and if we really want to support windows we can do that in the future.

jlongster added a commit that referenced this pull request Feb 4, 2015
Implement lstripBlocks and trimBlocks from Jinja2
@jlongster jlongster merged commit dafc9e3 into mozilla:master Feb 4, 2015
@magnusjt
Copy link
Contributor

magnusjt commented Oct 6, 2015

Will you be supporting windows-style line breaks? This issue had me confused for quite some time..

@carljm
Copy link
Contributor

carljm commented Oct 6, 2015

@magnusjt I'm not aware of anyone currently planning to implement support for Windows-style line breaks. I'd be happy to look at a pull request.

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

5 participants