Incompatible with retry

[neverfox]

I attempted to pair this library with retry to deal with those inevitable moments when Yahoo! responds with an error, like so:

getHistorical = (requestOptions, retryOptions) ->
  operation = retry.operation retryOptions or {}

  new Promise (resolve, reject) ->
    operation.attempt (currentAttempt) ->
      console.log "Attempt ##{currentAttempt}: #{requestOptions.symbol} from #{requestOptions.from} to #{requestOptions.to}"
      # Queue up Yahoo! request
      queue.add ->
        removeTokens(1).then ->
          historical requestOptions
      .spread (quotes, url, symbol) ->
        console.log "=== #{symbol} (#{quotes.length}) ==="
        console.log url
        console.log "#{JSON.stringify(quotes[0], null, 2)}\n...\n#{JSON.stringify(quotes[quotes.length - 1], null, 2)}"
        # Resolve main Promise if successful
        resolve quotes
      .catch (err) ->
        # Reject main Promise if maximum amount of retries has been reached
        reject operation.mainError()  if not operation.retry err

exports.getPrices = (symbol, start, end, period) ->
  getHistorical
    symbol: symbol
    from: start
    to: end
    period: period
  ,
    #10 retries / day
    retries: 10
    factor: 2.08679
    minTimeout: 60 * 1000
  .then (quotes) ->
    _.map quotes, (quote) ->
      date: convertTimestamp quote.date
      close: quote.close
      adj_close: quote.adjClose

What I discovered in my logs was something like this:

» 06:30:22.050 2014-10-01 13:30:21.640671+00:00 app worker.1 - - Attempt #1: VBR from 2012-10-01 to 2014-10-01 Context
» 06:31:50.947 2014-10-01 13:31:50.650148+00:00 app worker.1 - - Attempt #2: VBR from 1349049600000 to 1412121600000 Context
» 06:33:56.660 2014-10-01 13:33:55.956010+00:00 app worker.1 - - Attempt #3: VBR from 1349049600000 to 1412121600000 Context
» 06:38:17.644 2014-10-01 13:38:17.335977+00:00 app worker.1 - - Attempt #4: VBR from 1349049600000 to 1412121600000 Context
» 06:47:23.173 2014-10-01 13:47:22.775720+00:00 app worker.1 - - Attempt #5: VBR from 1349049600000 to 1412121600000 Context
» 07:06:21.077 2014-10-01 14:06:20.674806+00:00 app worker.1 - - Attempt #6: VBR from 1349049600000 to 1412121600000 Context
» 07:45:55.519 2014-10-01 14:45:55.122230+00:00 app worker.1 - - Attempt #7: VBR from 1349049600000 to 1412121600000 Context
» 09:08:30.310 2014-10-01 16:08:29.995148+00:00 app worker.1 - - Attempt #8: VBR from 1349049600000 to 1412121600000 Context
» 12:00:50.007 2014-10-01 19:00:49.613432+00:00 app worker.1 - - Attempt #9: VBR from 1349049600000 to 1412121600000 Context
» 18:00:26.680 2014-10-02 01:00:26.288284+00:00 app worker.1 - - Attempt #10: VBR from 1349049600000 to 1412121600000 Context
» 06:30:52.685 2014-10-02 13:30:52.277698+00:00 app worker.1 - - Attempt #11: VBR from 1349049600000 to 1412121600000 Context

All of my retries were trying to use timestamps rather than date strings, and failing as a result. After scouring my code, I took a peek at the source for node-yahoo-finance and realized that it was directly manipulating the options object that was originally passed in:

  if (_.isString(options.from) && !_.isEmpty(options.from)) {
    options.from = moment(options.from);
    assert(options.from.isValid(), '"options.from" must be a valid date string.');
  } else {
    assert(_.isDate(options.from) || _.isUndefined(options.from) || _.isNull(options.from),
           '"options.from" must be a date or undefined/null.');
    if (_.isDate(options.from)) {
      options.from = moment(options.from);
    }
  }

  if (_.isString(options.to) && !_.isEmpty(options.to)) {
    options.to = moment(options.to);
    assert(options.to.isValid(), '"options.to" must be a valid date string.');
  } else {
    assert(_.isDate(options.to) || _.isUndefined(options.to) || _.isNull(options.to),
           '"options.to" must be a date or undefined/null.');
    if (_.isDate(options.to)) {
      options.to = moment(options.to);
    }
  }

So I would like to propose a change that would not modify the options object, because I think being able to pair node-yahoo-finance with something like retry is powerful and useful. Plus I think it makes good sense to be able to trust that an object that you create isn't being modified outside of your vision. I would be willing to do a PR but if you have any guidance on how you would like to handle it, please let me know.

[pilwon]

@neverfox Great proposal. Does this commit 7c3551aaba2ba84318ae137bf8dd3f55be7cbc11 (yahoo-finance@0.2.9) help with the problem you encountered?

[neverfox]

@pilwon I'll check it out tonight. Thanks for the fast work.

[neverfox]

I can confirm that it works as expected in that context now.

[pilwon]

@neverfox :+1:

[neverfox]

Making the jump from 0.1.x to 0.2.x, I did have to refactor to adjust for the fact that a single symbol historical request no longer returns the symbol and url along with the quotes. What was the thought behind that change?

[neverfox]

And thank you!

[pilwon]

@neverfox To keep it more simple and straightforward. the symbol(s) information is already known to the caller, therefore it is redundant to return this information back to the caller. The new version of this library also became a bit smarter with fetching and transforming data, therefore url is no longer needed to be exposed. If you still want to see the url, you can always run your app with DEBUG=yahoo-finance:* env var.