praekeltfoundation / jspot

Extracts gettext strings from Javascript files into pot files
BSD 3-Clause "New" or "Revised" License
12 stars 2 forks source link

Add handlebars extractor #19

Closed JSteunou closed 10 years ago

JSteunou commented 10 years ago

First step to issue #11

Missing tests and doc but working nice.

The gettext should be registered in Handlebars as an helper, and all derivate. Suppose you are using i18n

var Handlebars = require('handlebars');
var i18n = require('./i18n'); // your own gettext, maybe build upon Jed

Handlebars.registerHelper('i18n', function(context) {
    // arguments = in template param + a handlebars hash
    // this = in template context
    return i18n.apply(i18n, Array.prototype.slice.call(arguments, 0, arguments.length - 1)));
});

Handlebars.registerHelper('i18n.ngettext', function() {
    // arguments = in template param + a handlebars hash
    // this = in template context
    return i18n.ngettext.apply(i18n, Array.prototype.slice.call(arguments, 0, arguments.length - 1)));
});

In your template

<div>{{i18n "I'm singular" }}</div>
<div>{{i18n.ngettext "dog" "dogs" count }}</div>
justinvdm commented 10 years ago

@JSteunou woah, thanks for the PR! This is looking great.

Sorry for the lack of activity in jspot, as usual, we're pretty busy. I was thinking that we weren't yet ready to support more extractors, but looking at the PR, it looks like you've been able to support a new extractor type quite nicely :)

I'm a little concerned about jspot becoming bloated with too many extractor types, ideally I'd like to keep only the most commonly used ones bundled in the actual project, and allow alternative ones to be supported as plugins. That said, handlebars is probably a very common case, so I think it would make sense to have this one be bundled in.

Once tests have been added, I'd be happy to give this PR a review so we can get it merged :)

JSteunou commented 10 years ago

@justinvdm dont blame yourself, I was quite busy too, and came back to you only now, months after planing to add handlebars support.

About extractors, I completely agree. I hesitate to add handlebars like I did. It adds handlebars to the project dependencies, and each extractor could make it grows. A plugin system could be nice, but I did not have the time to write one :p

About tests, I dont know yet how they could cover all cases. I might write some pretty simple and add others to prevent regression if some bugs are found later. I did a big template with some tortuous cases to generate a pretty big handlebars AST to work on when I started to write this extractor. It could be a start.

After that I plan to write a grunt jspot plugin, but I will wait for this to be reviewed merged & published ;)

justinvdm commented 10 years ago

@JSteunou Sounds like a plan.

If we could test similar things (where relevant) to what we test in js.test.js, I think that'd be great, unless there are reasons that I'm missing that will make this hard to do.

JSteunou commented 10 years ago

The hard part is covering all possible tags and tags inclusion configuration. What if a #unless tag inside a #each inside a else made a non covered case?

It's quite odd and should not happen, but I had a surprise with the else tag, handled with a inverse property in the AST, that I nearly missed.

justinvdm commented 10 years ago

@JSteunou Ah, I see what you mean. Short of testing all the possible possible tag combinations, I can't think of a way we'd be able to do that. Maybe we could just have a test for checking that the extractor works for nested cases (maybe the test can have some heavy nested case, with the gettext call inside an #if, which is inside a #unless, which is inside an else, which is inside an #each). I'd be happy with that if you are?

JSteunou commented 10 years ago

@justinvdm tests added and commits rebased to clean up the logs ;)

justinvdm commented 10 years ago

\o/ awesome! will take a look as soon as I get a chance

justinvdm commented 10 years ago

This looks great :)

I'm not familiar with parsing the handlebars AST, but the extractor is really well tested and its implementation is clean and reads well. I'm going to go ahead and merge.

Thanks again for adding support for this!

justinvdm commented 10 years ago

@JSteunou just published v0.3.0 with this feature included :)

JSteunou commented 10 years ago

You're welcome, glad I could help on this project.