search

1Password / connect-sdk-python

Python SDK for 1Password Connect
https://developer.1password.com/docs/connect
MIT License
200 stars 30 forks source link

Add return types to client functions. #68

Closed melwil closed 10 months ago

melwil commented 1 year ago

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.

image
melwil commented 10 months ago

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 commented 10 months ago

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.

melwil commented 10 months ago

Same for the return types needing Union. I should really take another stab at getting 3.8 to work properly 😅

melwil commented 10 months ago

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?

williamhpark commented 10 months ago

@melwil Thank you for catching and making those changes! Hm interesting, I'm currently looking into what's causing that to fail.

williamhpark commented 10 months ago

@melwil Re-running the jobs did the trick! Seems like it was just a bit flaky.

Unfortunately, merging is currently blocked because commit https://github.com/1Password/connect-sdk-python/pull/68/commits/596465756399b7b7b7f48eb69b6b9931b4d939e3 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?

melwil commented 10 months ago

did you perhaps use a different signing key for the unverified commit that wasn't configured in Git?

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? 🤔

williamhpark commented 10 months ago

@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.

melwil commented 10 months ago

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.

melwil commented 10 months ago

It looks good to me, apart from that one mistake with the imports.

codecov-commenter commented 10 months ago

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (5f8ffd8) 77.05% compared to head (094116e) 77.13%.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ 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 ``` | [Files](https://app.codecov.io/gh/1Password/connect-sdk-python/pull/68?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | | |---|---|---| | [src/onepasswordconnectsdk/async\_client.py](https://app.codecov.io/gh/1Password/connect-sdk-python/pull/68?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL29uZXBhc3N3b3JkY29ubmVjdHNkay9hc3luY19jbGllbnQucHk=) | `65.16% <100.00%> (+0.22%)` | :arrow_up: | | [src/onepasswordconnectsdk/client.py](https://app.codecov.io/gh/1Password/connect-sdk-python/pull/68?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL29uZXBhc3N3b3JkY29ubmVjdHNkay9jbGllbnQucHk=) | `63.95% <93.54%> (+0.79%)` | :arrow_up: |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

williamhpark commented 10 months ago

@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!