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

Fix several bittrex API issues #181

Merged
merged 1 commit into from
May 1, 2018

Conversation

firepol
Copy link

@firepol firepol commented Apr 19, 2018

No description provided.

"""Return the account wallet."""
payload = kwargs
if currency:
payload = {'currency': currency}
payload.update(kwargs)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good attempt at fixing this, but better would be:

payload = kwargs
if currency:
    payload.update({'currency': currency})

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even better I think it's to update kwargs and pass kwargs to the params instead of a new variable called payload. I updated the entire file accordingly.

@@ -103,13 +102,12 @@ def cancel_order(self, *order_ids, **kwargs):
return results if len(results) > 1 else results[0]

@format_with(BittrexFormattedResponse)
def wallet(self, *args, currency=None, **kwargs): # pylint: disable=arguments-differ
def wallet(self, currency=None, **kwargs):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See Issue #183 for the reason *args and **kwargs need to stay.
Moving the pylint comment above the line might actually be prettier.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but *args must stay before currency or changing the order is ok? Because I was assuming, for consistency, that *args and **kwargs ideally should always be the last parameters of the methods. Not put something in the middle like the currency in this case...

Fix withdraw & *_history methods (they are private: authenticate=True).
Add pair check in the trade_history method.
Various DRY-ups and clean-ups.
@firepol
Copy link
Author

firepol commented Apr 24, 2018

Updated based on your feedback, just def wallet(self, *args, currency=None, **kwargs) doesn't convince me enough. I'd like to have *args and **kwargs at the end of the method signature, like this:

def wallet(self, currency=None, *args, **kwargs)

Any reason to put currency between them? It just makes it look inconsistent.

@deepbrook
Copy link
Collaborator

Regarding the order of arguments:
It's a code-style thing - first come positionals, then keyword arguments.

@deepbrook deepbrook merged commit ba0ecb1 into Crypto-toolbox:dev May 1, 2018
@firepol firepol deleted the bittrex-fixes branch May 1, 2018 06:48
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.

None yet

2 participants