Add return types to client functions. #68
Conversation
There was a problem hiding this comment.
That's a great improvement to the sdk! We definitely should use the power of types.
Unfortunately, your change in deserialize function usage won't work if pass the type as it is and not as a string. (aka list[File] instead of "list[File]" string)
I left a single comment about that, but actually, all the deserialize usages should be reverted in order to make it work.
|
Thanks for the review, @williamhpark ! I had missed the fact that an async client had been added. Added return types to that as well, by the way 😉 |
melwil
force-pushed
the
improvement/return-types-for-client
branch
from
9c3d002 to
5eeab2f
Compare
|
I think the errors in the test run were related to the typing of list and dict, the typing style changed in 3.9 and so the tests in 3.8 failed with the newer syntax. |
|
Same for the return types needing Union. I should really take another stab at getting 3.8 to work properly 😅 |
|
Seems like what fails now is uploading the code coverage generated by the test. Not sure if my branch impacted this? Would a merge with main be needed or something? |
|
@melwil Thank you for catching and making those changes! Hm interesting, I'm currently looking into what's causing that to fail. |
|
@melwil Re-running the jobs did the trick! Seems like it was just a bit flaky. Unfortunately, merging is currently blocked because commit 5964657 was unverified. I noticed that your later commits were successfully signed - did you perhaps use a different signing key for the unverified commit that wasn't configured in GitHub? |
That commit was made a long time ago, I didn't have signing keys for github set up back then 😅 I could check if I can manage to sign it retroactively. Alternatively, if I turn off strict mode, would that help? 🤔 |
|
@melwil I see! IIRC, when this had happened to me in the past, I ended up having to overwrite my unsigned commits. This Stack Overflow post can hopefully provide some more insight. |
…to get_file_content return types.
melwil
force-pushed
the
improvement/return-types-for-client
branch
from
8ae6b03 to
15a8f05
Compare
|
Ok, better run the tests one more time for this, bit messy with the rebase considering the age of that first commit. Looking over the changes again might be a good idea too. But all signed now. |
|
It looks good to me, apart from that one mistake with the imports. |
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #68 +/- ##
==========================================
+ Coverage 77.05% 77.13% +0.07%
==========================================
Files 27 27
Lines 1931 1933 +2
==========================================
+ Hits 1488 1491 +3
+ Misses 443 442 -1
☔ View full report in Codecov by Sentry. |
|
@melwil I took a look again and there was one thing that I think I missed in my initial review that I left a comment on. Besides that, everything looks good to me! |
This PR aims to introduce type hints to return values for the client.
The changes are simple enough, simply adding the types as return type hints on each function in the client.
Working with the sdk is somewhat cumbersome, since I work in a typed project. The return types from the sdk are not great [see attachment], and properly resolving the typing of the responses from the sdk client should not really be necessary.
I've made a suggestion of how it could be done, but I had some trouble installing 3.7 to verify that this works as expected. I'm not sure if there were good reasons for leaving the types out, or if resolving the models leads to other problems.