search

gferrin / bitfinex

node.js wrapper for bitfinex cryptocurrency exchange
25 stars 20 forks source link

fix #14 and #15 #16

Closed dutu closed 9 years ago

dutu commented 9 years ago

any chance to have this merged and new module rev released? My travis is currently failing https://travis-ci.org/dutu/cryptox/ due to the dependencies.

The changes introduced have passed all tests, but feel free to test more if needed. If you can't review and merge soon, pls let me know and I'll remove the dependency on this module for now. Thanks

gferrin commented 9 years ago

Yeah, I appreciate your pull request and will get it merged today.

gferrin commented 9 years ago

I like you changes, but I don't like having a past_trades and a mytrades function doing exactly the same thing but I like you new implementation for mytrades so I'm going to take the code for that and continue calling is past_trades

dutu commented 9 years ago

i did it as separate function to keep backward compatibility of the module, so that whoever is using past_trades today can still continue using it

dutu commented 9 years ago

btw, Bitfinex calls the method mytrades :)

if you introduce API changes which are not backwards compatible, pls it would be good incrementing the major number of the module revision (1.x.x)

dutu commented 9 years ago

yes, i did it for MINOR which is for API changes that are backwards compatible. for non-backward compatible changes is recommended that you increase the MAJOR. Reference: http://semver.org/

gferrin commented 9 years ago

Yeah your right - I reverted to your pull request and published it exactly as version 0.4.1 I then went back to mine and published it as 1.0.0 - Thanks for your contributions!