pugjs / then-pug

**EXPERIMENTAL** Async promise based Jade
42 stars 5 forks source link

Each method needs documenting #2

Closed jeromew closed 10 years ago

jeromew commented 10 years ago

(simply reporting what's left to be done)

jeromew commented 10 years ago

@ForbesLindesay I added documentation to the project can you take a look and tell me what you think ? https://github.com/then/then-jade/commit/f8140cdc9590299ad6151b83ac78140aa9562cab

I did not check by running an example + I am not totally familiar with barrage but I did not fully understand the render and renderFile functions :

function renderFile(path, options, callback) {
    return Promise.from(null).then(function () {
      return renderFileStreaming(path, options).buffer('utf8', callback);
    });
  }

shouldn't it be either

// callback style
function renderFile(path, options, callback) {
     renderFileStreaming(path, options).buffer('utf8', callback);
}

or

// promise styme
function renderFile(path, options) {
    return Promise.from(null).then(function () {
      return renderFileStreaming(path, options).buffer('utf8');
    });
  }

or did you mean to make callback optional and have the best of both worlds (callback style when callback is set and Promise when callback is not set)

depending on your response I will improve the documentation on those 2 functions.

ForbesLindesay commented 10 years ago

It should actually have been:

// promise styme
function renderFile(path, options, callback) {
  return Promise.from(null).then(function () {
    return renderFileStreaming(path, options).buffer('utf8');
  }).nodeify(callback);
}

which does indeed make the callback optional and returns a promise if there is no callback.

barrage just extends streams with a few extra methods (e.g. the .buffer(encoding) method). Other than those extra methods it's exactly like the builtin stream module.

ForbesLindesay commented 10 years ago

The reason for wrapping in return Promise.from(null).then(function () { .... }) is to trap any synchronous exceptions and turn them into asynchronous exceptions. The docs so far look good. We'll want to duplicate most of the same info in the README file as well.

jeromew commented 10 years ago

documentation seems ok. tests are broken now apparently when doing a fresh "npm install" (probably recent modifications on jade).