Version 2.0 - Githubissues

kmadac / bitstamp-python-client

Python package to communicate with bitstamp.net

MIT License

143 stars 70 forks source link

Version 2.0 #8

Closed SmileyChris closed 10 years ago

SmileyChris commented 10 years ago

Refactored code, making it much more DRY
Better behaviour of just raising exception on a Bitstamp API error
Backwards compatible (old lowercase class names still behave the same)
Fixed tests
Tests now use mock API calls
Tox tests for easy cross-python testing
Removed unsupported bitinstant_reserves call
PEP8 tidy up, and docstrings galore

certik commented 10 years ago

I like the changes in general, especially the PEP8 and raising an exception on error.

Few things I noticed that are a matter of taste:

Using rst instead of md for README (I prefer md these days)
Using setuptools instead of distutils (I prefer to avoid setuptools unless I absolutely have to)

But otherwise +1 from me.

SmileyChris commented 10 years ago

rst/md is a definite taste. I repainted the bikeshed so I used my favourite colour :) Fine with me if the author wants to switch that back though.

I really feel like we're getting past the "no setuptools" stage these days (e.g. who doesn't use pip now?). It also is a requirement so that tox can be used.

certik commented 10 years ago

(Since you asked, I don't use pip. I use Conda or Hashdist.)

Anyway, +1 from me to merge as it is.

kmadac commented 10 years ago

Hi Chris, Your work looks great. I really like new tests and all refactored code looks much more mature now. Your python skills are much greater than mine, so please give me some time to review it.

SmileyChris commented 10 years ago

Fixed a bug I was encountering with nonce generation.

kmadac commented 10 years ago

I reviewed and tested the code and I'm OK with merging PR to master. Thank you for your great contribution Chris.