Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Trade API #15

Merged
merged 1 commit into from
Aug 4, 2015
Merged

Implement Trade API #15

merged 1 commit into from
Aug 4, 2015

Conversation

elyobo
Copy link
Contributor

@elyobo elyobo commented Jul 31, 2015

No description provided.

@elyobo
Copy link
Contributor Author

elyobo commented Jul 31, 2015

@toloco Note that this requires changes to __call, as it strips all non-truthy arguments but update_trade explicitly requires values of 0 to remove some trade parameters. Passing 0 seems valid, and passing empty values (e.g. '') may also be desirable in some situations, so this seems like a
safe change.

I'm also not sure what sort of validation I should be doing on arguments coming in (e.g. maxId and count should always be things that will convert to an integer value); I've left them alone as they'll eventually return an API error if they're not valid, but we could be more proactive and immediately return an error when we get values that we know won't work.

@landscape-bot
Copy link

Code Health
Code quality remained the same when pulling 45328c3 on elyobo:implement-trades into 9eeb129 on toloco:master.

@elyobo elyobo force-pushed the implement-trades branch 2 times, most recently from df6b7cb to 3db1918 Compare August 3, 2015 06:45
Note that this requires changes to __call, as it strips all non-truthy
arguments but update_trade explicitly requires values of 0 to remove
some trade parameters.  Passing 0 seems valid, and passing empty values
(e.g. '') may also be desirable in some situations, so this seems like a
safe change.
@landscape-bot
Copy link

Code Health
Code quality remained the same when pulling 7b15d61 on elyobo:implement-trades into 9eeb129 on toloco:master.

@elyobo elyobo mentioned this pull request Aug 3, 2015
@elyobo
Copy link
Contributor Author

elyobo commented Aug 3, 2015

Implements #2

@toloco
Copy link
Owner

toloco commented Aug 4, 2015

Good to go!

toloco added a commit that referenced this pull request Aug 4, 2015
@toloco toloco merged commit 1f8a93e into toloco:master Aug 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants