albertosantini / node-rio

Integration with Rserve, a TCP/IP server for R framework
https://github.com/albertosantini/node-conpa
MIT License
176 stars 35 forks source link

Unify evaluate api #22

Closed albertosantini closed 8 years ago

albertosantini commented 9 years ago

The proposal is the unification of the evaluation APIs into one.

AS IS

function evaluate(cmd, options) { ... function sourceAndEval(filename, options) {... function bufferAndEval(buffer, options) { ...

TO BE

Signature: function evaluate(params) { ...

Default params:

params = {
    command: "",
    filename: "",

    entrypoint: "", 
    data: {},

    callback: function (err, res) {
        if (!err) {
            util.puts(res);
        } else {
            util.puts("Rserve call failed. " + err);
        }
    },

    host = "127.0.0.1",
    port = 6311,
    path = undefined,

    user = "anon",
    password = "anon"
}

Alias: function E(params) { ...

Notes

rio.E({
    command: "source('script.R')", // or library(myPackage)
    entrypoint: "getOptimalPortfolio",
    data: myData,
    callback: displayResponse
});

It is a convenient way to append something after the command, especially the stringify of data.

Constraints

Snippets extracted from the examples.

rio.E({command: "2+2"});
rio.E({
    filename: path.join(__dirname, "ex2.R")
    entrypoint: "getOptimalPortfolio",
    data: myData,
    callback: displayResponse
});
rio.E({
    host: myHost,
    port: myPort,
    user: myUser,
    password: myPassword,
    filename: path.join(__dirname, "ex3.R")
    entrypoint: "getOptimalPortfolio",
    data: myData,
    callback: displayResponse
});
rio.E({
    filename: path.join(__dirname, "ex5.R")
    entrypoint: "createDummyPlot",
    callback: getPlot
});
rio.E({
    entrypoint: "echo",
    data: myData,
    callback: printEcho
});
alexproca commented 9 years ago

I like the API as you describe it so far. What do you think about using promises instead of callback.

rio.E({
    entrypoint: "echo",
    data: myData
})
.then(function(data) {
   console.log(data);
})
.catch(function(error) { 
   console.log(error);
});
albertosantini commented 9 years ago

Very tempting. :)

One main advantage it would be a better error handling.

Anyway I prefer a base and minimal approach, using callbacks: they are a fundamental building block, letting the user how to handle the async flow.

Finally the user can use a promisification step.

See, for instance, bluebird promisification.

I would not add complexity, choosing an opinionated Promises library, or another abstract layer: rio vision aims to simplicity and lightweight.

albertosantini commented 9 years ago

Alias is rio.e(params) { ... due to eslint new-cap rule, a function with a name starting with an uppercase letter should only be used as a constructor.

I prefer not to disable it.

tamaracha commented 9 years ago

Sorry if this is the wrong place to ask. I tried to promisify rio.evaluate with bluebird, but it doesnMt work, because bluebird expects the callback as the last argument, whereas rio.evaluate's callback is an option in the options hash. Many API's have the options as an intermediate argument and the callback as the last one. This would be nice in terms of unification, while keeping callback style.

albertosantini commented 9 years ago

Nice point! :)

After changing my working directory

function evaluate(params, callback) {
    var cmd = params.command || "",
        filename = params.filename || "",
        entrypoint = params.entrypoint || "",
        cb = params.callback || callback,
        options = params;

    options.callback = cb;
...
// TODO: fix default callback in sendAction

I tried

var Promise = require("bluebird");
var rio = Promise.promisifyAll(require("../lib/rio"));

rio.eAsync({command: "pi / 2 * 2"}).then(function(res) {console.log(res)}); 
// => 3.14...

@tamaracha would be enough for your use cases?

tamaracha commented 9 years ago

Nice, yet another way :-)

I solved it myself by manually promisifying rio.evaluate:

var rio=require("rio");
rio.evaluateAsync=function(command,options){
  return new Promise(function(resolve,reject){
    if(!options){var options={};}
    options.callback=function(err,result){
      if(err){return reject(err);}
      return resolve(result);
    };
    return rio.evaluate(command,options);
  });
};

Now, in express, I can do something like:

return rio.evaluateAsync(req.params.rio)
.then(function(data){
  return res.send(data.toString()); // or whatever
},function(e){
  return res.sendStatus(500);
});

I'm comfortable now, thanks for the quick reply.

albertosantini commented 9 years ago

Ok. I take a step away from the change.

Thanks about the snippet. I will add the example code in the README in the section Promisifying. :)

albertosantini commented 9 years ago

@tamaracha A few months ago... https://github.com/albertosantini/node-rio/commit/34d517597e28de8553b89aaa94f454f7f13676a5

albertosantini commented 8 years ago

dev-2.x branch merged.