search

enyo / node-tvdb

A node library for thetvdb.com
26 stars 12 forks source link

A couple questions #19

Closed mlazaric closed 9 years ago

mlazaric commented 10 years ago
  1. Is https://github.com/enyo/node-tvdb/blob/develop/test.js used anywhere any more?
  2. https://github.com/enyo/node-tvdb/blob/develop/CONTRIBUTING.md#coffeescript says you shouldn't commit JS, not sure if I'm misinterpreting and you just mean there shouldn't be any JS only PRs or if no JS should ever be commited (ie you shouldn't compile index.coffee and commit the index.js WITH everything else)?
  3. The custom formatting of the fields of episodes and show, shouldn't you pass along all the info? There are some keys you don't format so they don't reach the callback. A good solution might be to have an option to skip formatting or just adding all the keys to the keyMapping object. Could someone clarify why there isn't a formatActor and a formatBanner? Do they not need any formatting, because if they do a more consistent solution might be to separate the formatting into functions like you did with episode and show.
  4. Would you accept a PR to add promises as an alternative to callbacks? I'm not sure if it would fit into the module or if it should be up to the user (of the module) to promisify it. But if you do think it might fit into the module itself, it should be rather easy to do.
  5. Not a question, more of an observation: the readme needs to be updated in a few places like https://github.com/enyo/node-tvdb/tree/develop#find-a-tv-show the number of optional values has increased and some function don't seem to be listed in the readme at all.

Thank you for your time, please correct me if I'm wrong on anything.

enyo commented 9 years ago

Hey, sorry for the late reply

  1. Nope. test.js can be deleted. Was a quick test for me before I wrote the tests.
  2. To make it easier to review changes, I'd like only Coffeescript changes to be included in a PR. I will then compile everything before I release a new version.
  3. I just bring those fields in proper JavaScript formatting. If things are missing, they should be added :)
  4. Definitely! I would go with Q though.
  5. I'm happy for contributions :)
mlazaric commented 9 years ago

Would you prefer each of these be addressed in a separate commit (but same PR) so you can cherry pick the commits? Do you want to stick with callbacks as the default and wrap them in promises as an alternative or the other way around with promises being the default? Also is there a reason the keys that are always present are separated from the optional keys (the ones in the keyMapping object)?

Sorry for kind of necroposting but didn't want to create a new issue just to ask this.

enyo commented 9 years ago

I prefer separate smaller commits. I would just remove callback functionality and go promises all in.

The only reason for separating, was because optional parameters need this code:

_.each keyMapping, (trgKey, srcKey) ->
  srcValue = actor[srcKey]
  formattedActor[trgKey] = srcValue if srcValue

and required ones, don't. So I just set the required ones directly. (I wouldn't use underscore for that anymore though... it's so simple to do such a loop with coffeescript)

No problem with posting here!