update for api v2.0#7
update for api v2.0#7galperins4 wants to merge 23 commits intoArkEcosystem:developfrom galperins4:develop
Conversation
* Delete __init__.py * initial API v2 classes * updated ark.py new config.py added api v2 methods to ark.py added config.py to manage port switching for connections (e.g., p2p, public API / webhooks) * Update README.md * update setup to remove p2p folder * Delete config.py * update ark to move config in as method * Update ark.py * update ark.py to fix typo * fix typo in switch config * update for new api reference for api v2 * update api.py remove json error message as it causes key error with v2 api * fix typo * fix typos * fix typo * fix typo * typo * added missing parameters * updated items completed
| git clone https://github.com/ArkEcosystem/python-client | ||
| cd python-client | ||
| sudo python3 setup.py install | ||
| pip3 install setuptools |
There was a problem hiding this comment.
no need for pip3 install setuptools
roks0n
left a comment
There was a problem hiding this comment.
There's also a lot of flake8 errors in Travis
| class Peer2(API): | ||
|
|
||
| def peer(self, ip): | ||
| return self.get(f'api/v2/peers/{ip}') |
There was a problem hiding this comment.
this syntax only works in py3.6, we are supporting 3.4+
There was a problem hiding this comment.
Ok, I'll change them all to use the .format() nomeclature
| raise NotImplementedError() | ||
| return Vote2(self) | ||
|
|
||
| def switchConfig(self, service, nethash): |
There was a problem hiding this comment.
Can you follow the official guidelines (check naming section). I'd like us to follow official Python guidelines so we follow the standards.
There was a problem hiding this comment.
I ended up using camelcase for functions as needed to be consistent. Let me know if I should rename
There was a problem hiding this comment.
camelcase is used for classes/exceptions. For methods/functions you should use lowercase snake case.
| import requests | ||
|
|
||
|
|
||
| class API(ABC): |
There was a problem hiding this comment.
there's no need for an abstract base class
| def delegate(self, id): | ||
| return self.get(f'api/v2/delegates/{id}') | ||
|
|
||
| def delegates(self, parameters=None): |
There was a problem hiding this comment.
Instead of accepting dictionary for parameters, why not specify which args/kwargs can user pass in?
There was a problem hiding this comment.
I wanted the API calls to be as flexible and clean as possible. Also if any changes are made in the future to API inputs it won't break the API call and then we can just focus on maintaining the docstrings
galperins4
left a comment
There was a problem hiding this comment.
Made all the changes as requested
|
Can you also write some tests for this? use pytest for testing |
|
Will add this as a to do item for future releases |
galperins4
left a comment
There was a problem hiding this comment.
added snake_case as needed
galperins4
left a comment
There was a problem hiding this comment.
fixed the remaining flake8 errors
galperins4
left a comment
There was a problem hiding this comment.
fixed more flake8 errors
|
Sorry, but need to close this PR as we're going to restructure the client to be somewhat similar to other implementations of ark client. Thank you for your contributions so far. Until further notice (until we're happy with the base of the client) all future contributions will be closed and not merged in: https://github.com/ArkEcosystem/python-client#contributions-are-closed |
Please see https://docs.ark.io/docs/contributing for details before opening your pull request.