Mustache.js bug fix and test improvements from Twitter #81

Merged
merged 36 commits into from Sep 18, 2011

Conversation

Projects
None yet
5 participants
@bcherry
Contributor

bcherry commented Feb 21, 2011

Hello, again!

This pull request is nearly identical to the last one we submitted. The main difference is that I've removed all of the i18n stuff, so we can get the bug fix upstream. I'll follow up with the i18n stuff later.

  1. Fixed a bug we uncovered, where the content of section tags would get rendered twice, allowing Mustache injection. I'd be happy to provide more details on this, but we've also added a regression test to the suite in our fork.
  2. I've added Rhino as a test runner, in addition to running with jsc. We actually found some subtle regex-related bugs that caused failures in Mustache in Rhino and v8 only, so more coverage here seemed warranted. In the longer run, it would be nice to be able to also run the suite on a web page with QUnit or something, so you can try it in all browsers you'd like.
  3. I've added documentation to the README about how to run the test suite, and how to add new tests.

For now, we've changed the version to "0.3.1-dev-twitter" to make it clear in our codebase, but we'd love to integrate with the main branch.

Thanks!

@bcherry

This comment has been minimized.

Show comment
Hide comment
@bcherry

bcherry Mar 4, 2011

Contributor

Any chance we could get this stuff pulled upstream?

Thanks!

Contributor

bcherry commented Mar 4, 2011

Any chance we could get this stuff pulled upstream?

Thanks!

@snostorm

This comment has been minimized.

Show comment
Hide comment
@snostorm

snostorm Apr 14, 2011

+1 on closing the bug. I remember coming across this a number of months back, though we just updated our templates to get around it at the time.

+1 on closing the bug. I remember coming across this a number of months back, though we just updated our templates to get around it at the time.

@bcherry

This comment has been minimized.

Show comment
Hide comment
@bcherry

bcherry Apr 17, 2011

Contributor

Hello again! Any update?

Contributor

bcherry commented Apr 17, 2011

Hello again! Any update?

@janl

This comment has been minimized.

Show comment
Hide comment
@janl

janl Jun 10, 2011

Owner

Hey, sorry for the delay, I will have a look at this soon!

Owner

janl commented Jun 10, 2011

Hey, sorry for the delay, I will have a look at this soon!

@janl

This comment has been minimized.

Show comment
Hide comment
@janl

janl Jun 11, 2011

Owner

Hey, this looks great!

I'm happy to merge this if you could add a check if the various JS engines are actually installed.

I'd volunteer, but I'm unclear on how to integrate this with rspec.

I would like to support js (SpiderMonkey), jsc (JavaScriptCore) and rhino and show that the tests are run in the respective runtime:

> rake
Testing mustache.js in SpiderMonkey:
......................................................................
Done

Testing mustache.js in JavaScriptCore:
......................................................................
Done

Testing mustache.js in Rhino:
......................................................................
Done

with either of them missing if the respective engines can't be found and maybe an error message if no engine is found.

Owner

janl commented Jun 11, 2011

Hey, this looks great!

I'm happy to merge this if you could add a check if the various JS engines are actually installed.

I'd volunteer, but I'm unclear on how to integrate this with rspec.

I would like to support js (SpiderMonkey), jsc (JavaScriptCore) and rhino and show that the tests are run in the respective runtime:

> rake
Testing mustache.js in SpiderMonkey:
......................................................................
Done

Testing mustache.js in JavaScriptCore:
......................................................................
Done

Testing mustache.js in Rhino:
......................................................................
Done

with either of them missing if the respective engines can't be found and maybe an error message if no engine is found.

@bcherry

This comment has been minimized.

Show comment
Hide comment
@bcherry

bcherry Jul 28, 2011

Contributor

Hey! Looks like my gmail "bulk" filter has been catching github emails, so I never caught your response. Yeah, I can definitely look into supporting checking for the existence of those engines.

Contributor

bcherry commented Jul 28, 2011

Hey! Looks like my gmail "bulk" filter has been catching github emails, so I never caught your response. Yeah, I can definitely look into supporting checking for the existence of those engines.

@bcherry

This comment has been minimized.

Show comment
Hide comment
@bcherry

bcherry Jul 28, 2011

Contributor

Oh, and I've also pushed up two more commits, as you can see, that offer a handy performance optimization by caching the dynamic regexes. With this optimization, Tweet rendering on Twitter.com seems to be about 50-80% faster.

Contributor

bcherry commented Jul 28, 2011

Oh, and I've also pushed up two more commits, as you can see, that offer a handy performance optimization by caching the dynamic regexes. With this optimization, Tweet rendering on Twitter.com seems to be about 50-80% faster.

@janl

This comment has been minimized.

Show comment
Hide comment
@janl

janl Jul 28, 2011

Owner

@bcherry, awesomecake, can't wait to get this merged :)

Owner

janl commented Jul 28, 2011

@bcherry, awesomecake, can't wait to get this merged :)

@bcherry

This comment has been minimized.

Show comment
Hide comment
@bcherry

bcherry Aug 23, 2011

Contributor

Alright, this is good to go. I've added checks that the engines exist before running the specs, and a global fail if no engines were found at all, and I also made the output look exactly as your comment suggested. I have not added SpiderMonkey (that seemed out of scope, but I'd be happy to help with that separately). And it's all merged, etc. Let me know if you need anything else!

Contributor

bcherry commented Aug 23, 2011

Alright, this is good to go. I've added checks that the engines exist before running the specs, and a global fail if no engines were found at all, and I also made the output look exactly as your comment suggested. I have not added SpiderMonkey (that seemed out of scope, but I'd be happy to help with that separately). And it's all merged, etc. Let me know if you need anything else!

@abh

This comment has been minimized.

Show comment
Hide comment
@abh

abh Sep 11, 2011

@janl - any plans for merging this?

abh commented Sep 11, 2011

@janl - any plans for merging this?

@janl

This comment has been minimized.

Show comment
Hide comment
@janl

janl Sep 12, 2011

Owner

@abh I have all intentions to merge this, trying to make some time for it this week!

Owner

janl commented Sep 12, 2011

@abh I have all intentions to merge this, trying to make some time for it this week!

@janl janl merged commit 4fd1b8b into janl:master Sep 18, 2011

@janl

This comment has been minimized.

Show comment
Hide comment
@janl

janl Sep 18, 2011

Owner

Woooot, thanks!

Owner

janl commented Sep 18, 2011

Woooot, thanks!

@bcherry

This comment has been minimized.

Show comment
Hide comment
@bcherry

bcherry Sep 19, 2011

Contributor

Awesome, thanks!

Contributor

bcherry commented Sep 19, 2011

Awesome, thanks!

@lapidus

This comment has been minimized.

Show comment
Hide comment
@lapidus

lapidus Nov 9, 2011

@bcherry, @bobthecow:
+1: I'd love to see i18n support merged ...
Could anyone please add some details regarding a possible {'_': i18nHelper.translate} implementation?
And what would the translation method look like to support things like singular/plural, dot notation lookup of strings etc?

lapidus commented Nov 9, 2011

@bcherry, @bobthecow:
+1: I'd love to see i18n support merged ...
Could anyone please add some details regarding a possible {'_': i18nHelper.translate} implementation?
And what would the translation method look like to support things like singular/plural, dot notation lookup of strings etc?

@janl

This comment has been minimized.

Show comment
Hide comment
@janl

janl Nov 9, 2011

Owner
Owner

janl commented Nov 9, 2011

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