Skip to content
This repository has been archived by the owner on Feb 16, 2020. It is now read-only.

Update talib.js - fixed talib callback signature #924

Merged
merged 2 commits into from
Aug 7, 2017
Merged

Update talib.js - fixed talib callback signature #924

merged 2 commits into from
Aug 7, 2017

Conversation

metachris
Copy link
Contributor

Fixed callback signature of talib (needs err and result parameters, see https://github.com/oransel/node-talib).

Fixed callback signature of talib (needs `err` and `result` parameters, see https://github.com/oransel/node-talib).
@metachris metachris changed the title Update talib.js Update talib.js - fixed talib callback signature Aug 6, 2017
@metachris
Copy link
Contributor Author

Not 100% sure you can merge it like this.

  • How did that work before // was this callback signature introduced in a newer version of talib? Then we should update package.json accordingly.
  • Can result still contain the error property?

@askmike
Copy link
Owner

askmike commented Aug 6, 2017

Hey! The callback signature was changed as part in node-talib 1.0.3. If you manually install 1.0.2 this code will break (as well as everyone who has talib installed already and updates Gekko if this PR is in). I'll leave this PR open for a little bit until I've tested what happens when running this on talib 1.0.2.

@metachris
Copy link
Contributor Author

I see two ways of dealing with both the legacy talib and the new one:

  1. Change this execute method to accept two generic arguments arg1 and arg2 and test them so it works with both the old and new version of talib
  2. Have 2 different execute methods -- one for the old talib and one for the new?

What do you think?

@askmike
Copy link
Owner

askmike commented Aug 7, 2017

what about one proper execute method (the one in your PR), and one method that wraps the proper one and splits the error into it's own argument (in case of old talib verison)?

like so:

const executeCallback = function(err, result) { /* version in your PR */. }
const LegacyExecuteCallback = function(result) {
 var error = result.error;
 executeCallback.apply(this, [error, result]);
}

And we can use the proper one by testing talib.version (in combination with semver for example)?
What do you think?

@metachris
Copy link
Contributor Author

I think this is a good idea! I'll update the pull request.

@askmike
Copy link
Owner

askmike commented Aug 7, 2017

great thanks!

@askmike askmike changed the base branch from stable to develop August 7, 2017 16:52
@askmike askmike merged commit 23f9f1d into askmike:develop Aug 7, 2017
@askmike
Copy link
Owner

askmike commented Aug 7, 2017

I really like the style of composing (wrapping functions in functions), so that if we ever need to do some normalization of output (or input) or whatever, we only have to do that once.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants