hubot-archive / hubot-github-repo-event-notifier

Notifies about any GitHub repo event available via webhook.
https://www.npmjs.org/package/hubot-github-repo-event-notifier
57 stars 42 forks source link

Allow templating of notification msgs #18

Open patcon opened 10 years ago

patcon commented 10 years ago

17 ideally shouldn't have needed to be a PR

Would be nice to do something with mustache like we do here: https://github.com/gittip/hubot-heroku-deploy-notifier/blob/master/src/heroku-deploy-notifier.coffee

Since we have lots of diff event types, could just check an envvar derived from eventType (ie. HUBOT_GITHUB_EVENT_NOTIFIER_TEMPLATE_PULL_REQUEST)

Related: https://github.com/gittip/roobot/issues/55

parkr commented 10 years ago

I'd be :point_down: like a :dog: (in a good way) for templates, definitely an awesome idea. What templating engine would you use? I can't think of anything built-in to CoffeeScript. I wrote this a few years ago:

//
// Fills a string containing special templating syntax with the data provided.
//
// Ex:
//    tmpl = "${name} got a ${grade} in ${course}.";
//    data = { name: "John", grade: "B", course: "Plant Pathology" };
//    tmpl.template(data); // outputs "John got a B in Plant Pathology."
//
String.prototype.template = function(data){
    var regex = null, completed = this.toString();
    for(el in data){
        regex = new RegExp('\\${'+ el +'}', 'g');
        completed = completed.replace(regex, data[el]);
    }
    return completed.toString();
};
patcon commented 10 years ago

Rockin. That looks super-elegant, but to be honest, I don't know enough about writing templating engines to know what goes into the full packages and why they seem to be prefered. I'd personally air on the side using something like mustache.js or hogan.js -- do you feel strongly that they're bloated?

parkr commented 10 years ago

I also don't know enough about writng templating engines to know what goes into the full packages. I think the added weight is features. If we're just talking about outputting variables, I don't think we need an entire package. If we think we'd want some of the features of mustache, then I'd say go for it. Hogan looks way too bloated for our use-case here. Your call.