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

Template with escaped double braces. #862

Merged
merged 4 commits into from Aug 28, 2013

Conversation

Projects
None yet
3 participants
@wutali

wutali commented Feb 18, 2013

I added a test case for the issue #534 and rebase it.

@tseaver

This comment has been minimized.

Show comment
Hide comment
@tseaver

tseaver Feb 18, 2013

Member

The added test looks reasonable to me. Do we need to also test edge cases (e.g., unclosed escaped double braces, or ones which cross line breaks? I don't know whether Jinja2 allows such cases.

Member

tseaver commented Feb 18, 2013

The added test looks reasonable to me. Do we need to also test edge cases (e.g., unclosed escaped double braces, or ones which cross line breaks? I don't know whether Jinja2 allows such cases.

@wutali

This comment has been minimized.

Show comment
Hide comment
@wutali

wutali Feb 18, 2013

@tseaver thank for your review. I added some edged test cases for the escaped braces pattern.
But I could not decide which is better that raise an exception when there are no balanced braces or just output braces with the escaped characters. I implemented the second one on the above commit because it is difficult to detect a balanced braces, I think.
Is it OK? If there is some problem, I will fix it.

wutali commented Feb 18, 2013

@tseaver thank for your review. I added some edged test cases for the escaped braces pattern.
But I could not decide which is better that raise an exception when there are no balanced braces or just output braces with the escaped characters. I implemented the second one on the above commit because it is difficult to detect a balanced braces, I think.
Is it OK? If there is some problem, I will fix it.

@tseaver

This comment has been minimized.

Show comment
Hide comment
@tseaver

tseaver Apr 3, 2013

Member

I'm +1 on the merge (the added tests look fine to me), but am reluctant to do the merge myself, as I don't use Jinja2. Can somebody who does use it please review?

Member

tseaver commented Apr 3, 2013

I'm +1 on the merge (the added tests look fine to me), but am reluctant to do the merge myself, as I don't use Jinja2. Can somebody who does use it please review?

@mcdonc

This comment has been minimized.

Show comment
Hide comment
@mcdonc

mcdonc Apr 3, 2013

Member

@wutali thank you for the patch. It looks fine, and doesn't appear to break anything.

Could I trouble you to add your name to CONTRIBUTORS.txt in the root of the Pyramid proejct before I merge this?

Member

mcdonc commented Apr 3, 2013

@wutali thank you for the patch. It looks fine, and doesn't appear to break anything.

Could I trouble you to add your name to CONTRIBUTORS.txt in the root of the Pyramid proejct before I merge this?

@wutali

This comment has been minimized.

Show comment
Hide comment
@wutali

wutali Aug 28, 2013

@mcdonc Sorry, I missed your message. I notice it now.

No problem about the listing of CONTRIBUTORS.
Thank you. I'm glad to be listed on it.

wutali commented Aug 28, 2013

@mcdonc Sorry, I missed your message. I notice it now.

No problem about the listing of CONTRIBUTORS.
Thank you. I'm glad to be listed on it.

@mcdonc

This comment has been minimized.

Show comment
Hide comment
@mcdonc

mcdonc Aug 28, 2013

Member

Great. Can you add a commit to your fork adding yourself to the file?

Member

mcdonc commented Aug 28, 2013

Great. Can you add a commit to your fork adding yourself to the file?

@wutali

This comment has been minimized.

Show comment
Hide comment
@wutali

wutali Aug 28, 2013

@mcdonc I updated the file. thx.

wutali commented Aug 28, 2013

@mcdonc I updated the file. thx.

@mcdonc mcdonc merged commit 7aa3cb8 into Pylons:master Aug 28, 2013

@mcdonc

This comment has been minimized.

Show comment
Hide comment
@mcdonc

mcdonc Aug 28, 2013

Member

Thank you, merged!

Member

mcdonc commented Aug 28, 2013

Thank you, merged!

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