Skip to content

Adding proxy API's for etherscan #35

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jun 24, 2018
Merged

Adding proxy API's for etherscan #35

merged 2 commits into from
Jun 24, 2018

Conversation

agatsoh
Copy link
Contributor

@agatsoh agatsoh commented Jun 11, 2018

Hi I am extending the API's for proxy and will help for any other things in my free time. I will also write the remaining API's onceI find time.

@agatsoh
Copy link
Contributor Author

agatsoh commented Jun 11, 2018

@corpetty recommended that we stick to python3.6 What is your opinion ?

@corpetty
Copy link
Owner

corpetty commented Jun 21, 2018 •
edited
Loading

I agree that sticking with at least >= 3.6 is at least my preference.

I'd like to make sure that this doesn't break other's use of it. @AndreMiras @taylorjdawson do you need python versions before 3.6?

@corpetty corpetty mentioned this pull request Jun 21, 2018
Closed
@AndreMiras
Copy link
Contributor

Thanks for asking, my app is running on Python 3.6, but indeed normally libraries should have quite wide support to different python versions. And this is currently enforced with tox checking for 2 and 3 https://github.com/corpetty/py-etherscan-api/blob/f3cd702/tox.ini
But well Python2 should be dropped at some point so I'm fine if we drop it https://pythonclock.org

@taylorjdawson
Copy link
Contributor

@corpetty Nope, I am fine if we drop it too.

@AndreMiras
Copy link
Contributor

Please also drop Python2.7 from tox.ini otherwise Travis will fail https://travis-ci.org/corpetty/py-etherscan-api/jobs/390758838

@corpetty
Copy link
Owner

@agatsoh please remove 2.7 from tox.ini and travis.yaml and re-PR

@AndreMiras
Copy link
Contributor

I was about to provide a pull request to do so @corpetty but I saw you just did that in c43a473 and 03a4ab8
So we're fine to merge I think now :)

@taylorjdawson
Copy link
Contributor

@agatsoh The naming of 'check_keys_api' is somewhat ambiguous. I feel like adding comments would be valuable here (and eventually required w/ linting integration).

@corpetty corpetty merged commit c354599 into corpetty:master Jun 24, 2018
@taylorjdawson
Copy link
Contributor

@corpetty Why did that check fail?

@corpetty corpetty mentioned this pull request Jun 24, 2018
Closed
AndreMiras
AndreMiras reviewed Jun 24, 2018
View reviewed changes
tests/test_proxies.py
@@ -13,8 +13,8 @@ def test_get_most_recent_block(self):
api = Proxies(api_key=API_KEY)
# currently raises an exception even though it should not, see:
# https://github.com/corpetty/py-etherscan-api/issues/32
Copy link
Contributor

Choose a reason for hiding this comment

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

We forgot to remove that comment

Copy link
Contributor

Choose a reason for hiding this comment

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

I am currently working on cleaning up the code a bit. I'll remove that in my pr

Copy link
Contributor

Choose a reason for hiding this comment

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

Great thanks!
Also I'm not getting why Travis didn't fail on the linter when it should have complained. Running tox locally I see some valid complain.

Copy link
Contributor

Choose a reason for hiding this comment

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

So just remove the comment then?

Also, If the proxy apis fail you get this:

{
    jsonrpc: "2.0",
    id: 1,
    error: {
        code: -32602,
        message: "[some error message]"
    }
}

I don't think this is accounted for in this PR. However, I think #8 would fix this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't the linter still just pointing at the tests folder only?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes the two lines comments were with a self.assertRaises(EmptyResponse) to describe a bug. But the bug was fixed and assert removed in 06f7b04, but the comment was left. So yes basically just removing that two comment is enough.
Also you can delete the print, I don't think we need print in unit tests. Plus I think we should avoid making actual network calls, but actual mock it, but that's a different story.

I'm not sure regarding the rainy scenario. with error code and message.

Copy link
Contributor

Choose a reason for hiding this comment

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

True for the linter it's currently only tests folder, but that one is also failing. e.g. tests/test_proxies.py:4:1: F401 'etherscan.client.EmptyResponse' imported but unused https://github.com/corpetty/py-etherscan-api/blob/06f7b04/tests/test_proxies.py#L4 and other couple of more. So really not sure why Travis didn't catch these obvious ones.
They don't pop up when you run tox on your dev env?

Copy link
Contributor

Choose a reason for hiding this comment

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

I just ran flake8?

Copy link
Contributor

Choose a reason for hiding this comment

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

No I ran tox that rans flake8 for you.

tox
GLOB sdist-make: /home/andre/workspace/py-etherscan-api/setup.py
pep8 inst-nodeps: /home/andre/workspace/py-etherscan-api/.tox/dist/py_etherscan_api-0.7.0.zip
pep8 installed: certifi==2018.4.16,chardet==3.0.4,flake8==3.5.0,idna==2.6,mccabe==0.6.1,py-etherscan-api==0.7.0,pycodestyle==2.3.1,pyflakes==1.6.0,requests==2.18.4,urllib3==1.22
pep8 runtests: PYTHONHASHSEED='913872812'
pep8 runtests: commands[0] | flake8 tests/
tests/test_proxies.py:4:1: F401 'etherscan.client.EmptyResponse' imported but unused
tests/test_proxies.py:20:1: W391 blank line at end of file
tests/test_proxies.py:20:1: W293 blank line contains whitespace
ERROR: InvocationError: '/home/andre/workspace/py-etherscan-api/.tox/pep8/bin/flake8 tests/'
py3 inst-nodeps: /home/andre/workspace/py-etherscan-api/.tox/dist/py_etherscan_api-0.7.0.zip
py3 installed: atomicwrites==1.1.5,attrs==18.1.0,certifi==2018.4.16,chardet==3.0.4,idna==2.6,more-itertools==4.2.0,pluggy==0.6.0,py==1.5.3,py-etherscan-api==0.7.0,pytest==3.6.2,requests==2.18.4,six==1.11.0,typing==3.6.4,urllib3==1.22
py3 runtests: PYTHONHASHSEED='913872812'
py3 runtests: commands[0] | python -m unittest discover --start-directory=tests/
5848317
/usr/lib/python3.6/unittest/case.py:605: ResourceWarning: unclosed <ssl.SSLSocket fd=3, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=6, laddr=('192.168.1.130', 42938), raddr=('23.111.151.66', 443)>
  testMethod()
./usr/lib/python3.6/unittest/case.py:605: ResourceWarning: unclosed <ssl.SSLSocket fd=3, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=6, laddr=('192.168.1.130', 42940), raddr=('23.111.151.66', 443)>
  testMethod()
./usr/lib/python3.6/unittest/case.py:605: ResourceWarning: unclosed <ssl.SSLSocket fd=3, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=6, laddr=('192.168.1.130', 42942), raddr=('23.111.151.66', 443)>
  testMethod()
.
----------------------------------------------------------------------
Ran 3 tests in 2.913s

OK
_____________________________________________________________________________________________________________________________________ summary _____________________________________________________________________________________________________________________________________
ERROR:   pep8: commands failed
  py3: commands succeeded

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay just ran that. Got the same results. When should we switch flake8 to the whole directory?

@AndreMiras
Copy link
Contributor

AndreMiras commented Jun 24, 2018 via email

@taylorjdawson
Copy link
Contributor

We should have a slack/discord. Which would you guys prefer?

@AndreMiras
Copy link
Contributor

AndreMiras commented Jun 24, 2018 via email

@taylorjdawson
Copy link
Contributor

taylorjdawson commented Jun 24, 2018 via email

@AndreMiras
Copy link
Contributor

I was imagining the project to have his own gitter room. From what I remember, all it needs is the maintainer @corpetty to authenticate to https://gitter.im/ and pick a room. Then gitter bot will come with a pull request to add the gitter badge to the project README.
It's better if it's an "official room" created by the maintainer, because a bunch of hooks get added so we see pull requests and ticket updates directly in the room.

@corpetty
Copy link
Owner

done: https://gitter.im/py-etherscan/Lobby#

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
2 more reviewers

@AndreMiras AndreMiras AndreMiras left review comments

@taylorjdawson taylorjdawson taylorjdawson left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@agatsoh @corpetty @AndreMiras @taylorjdawson @stvenyin