lakenen / node-box-view

A node client for the Box View API
MIT License
13 stars 2 forks source link

switch 'retry' with callback so callback is last arg #10

Closed seanrose closed 10 years ago

seanrose commented 10 years ago

When I integrated the version with retry logic added, it broke the blocking package broadcast is using – it turns out that the package has a hard assumption that the callback of any function is always the final argument. A little digging around leads me to believe that always having the callback as the last arg is standard convention.

I can update the tests and everything also, but just wanted to make sure you're cool with this before submitting a PR…

lakenen commented 10 years ago

I think I'll actually rework the arguments a bit so that every method accepts zero, one or two args: ([opt], [cb]).

So then it will become something like:

client.documents.get({ id: 'someid', retry: true }, function (err, response) {
  // stuff
});

Or with other options, e.g.:

var options = {
  id: 'someid',
  retry: true,
  params: { is_downloadable: true }
};
client.sessions.create(options, function (err, response) {
  // stuff
});

// but, still keep the shorthand with just the id:
client.sessions.create('someid', function (err, response) {
  // stuff
});

Thoughts?

I totally agree that having the trailing retry is nonstandard and weird...

lakenen commented 10 years ago

Or maybe, to be more correct, only the actual options would be options, so in the case of sessions (since id is not optional):

var options = {
  retry: true,
  params: { is_downloadable: true }
};
client.sessions.create('someid', options, function (err, response) {
  // stuff
});