Add async support #83
Conversation
Undo accidental version bumps
This reverts commit a7ab393.
Codecov Report
@@ Coverage Diff @@
## master #83 +/- ##
==========================================
- Coverage 88.20% 87.41% -0.80%
==========================================
Files 25 32 +7
Lines 1450 1955 +505
==========================================
+ Hits 1279 1709 +430
- Misses 171 246 +75 |
|
Should be good to run the CI again. By the way, any idea why those two docstrings didn't show up when I ran |
There was a problem hiding this comment.
Hey @kevinheavey. Thank you for contributing. This PR looks great.
I think it would be better if we have clearer boundaries between the old synchronous client and the new async client so that this codebase is easier to maintain.
And that if someone decides to implement a client that supports websockets, they should not be cramming into the existing api.py file nor the existing integration tests.
solana/rpc/api.py
Outdated
| return resp | ||
|
|
||
|
|
||
| class AsyncClient(_ClientCore): # pylint: disable=too-many-public-methods |
There was a problem hiding this comment.
Would it be possible to split into three different files?
- solana/rpc/core.py
- solana/rpc/api.cpy (old synchornous client)
- solana/rpc/async_api.py (new async client)
There was a problem hiding this comment.
In the same spirit, I'll give AsyncToken its own file too
solana/rpc/providers/http.py
Outdated
|
|
||
|
|
||
| class AsyncHTTPProvider(AsyncBaseProvider, _HTTPProviderCore): | ||
| """Async HTTP provider to interact with the http rpc endpoint.""" |
|
|
||
|
|
||
| @pytest.mark.integration | ||
| def test_request_air_drop(stubbed_sender, test_http_client): | ||
| def test_request_air_drop(stubbed_sender, test_http_clients): |
There was a problem hiding this comment.
Can we test the async client separate from the old client? Having both synchronous and async tests in the same method makes maintenance more difficult.
There was a problem hiding this comment.
This is possible, but we'll have a lot of duplicated code without a big refactor of the test suite. Plus there are places where we just test that the response the async client received is the same as that of the sync client, and we'll lose that (without more refactoring).
To get this over the line I'll just duplicate all the old pre-async tests and make async versions of them. Your call if that's what you want
There was a problem hiding this comment.
Yes. Duplicate tests are fine, it's just less confusing. We can decide to refactor and reduce duplicates later.
|
Made those changes.
|
|
Forgot the linters, should work now |
|
Think pylint should be okay now |
|
Let's just disable |
|
Done. Got a good feeling about this one. Pylint is not complaining about anything I touched |
Closes #80
Summary of changes:
AsyncClientandAsyncTokenclasses.test_get_identitywhich I noticed wasn't testingget_identity. Instead it was redundantly testingget_genesis_hash.