search

coinbase / coinbase-advanced-py

The Advanced API Python SDK is a Python package that makes it easy to interact with the Coinbase Advanced API. The SDK handles authentication, HTTP connections, and provides helpful methods for interacting with the API.
https://docs.cdp.coinbase.com/advanced-trade/docs/welcome/
Apache License 2.0
109 stars 29 forks source link

Missing MyPy compatible type annotations #26

Closed StarringLara closed 8 months ago

StarringLara commented 8 months ago

Hello. It seems like there are no MyPy compatible type annotations provided by this package. Is that correct? IMO this is really important to support. If I run mypy on my code, it shows error: Skipping analyzing "coinbase": module is installed, but missing library stubs or py.typed marker [import-untyped] as well as note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports. If types are provided, and I'm just not using them right I will seek help elsewhere, I just wanted to get clarity on if they are provided or not and request that this be added if they are not.

I also believe it is important not just to provide a best guess of type stubs but to actually use types internally in the project and have them automatically statically validated to help keep them as accurate as possible.

github-actions[bot] commented 8 months ago

Thank you for reporting! If this is an SDK specific issue, we will look into it and get back to you soon. If this is an API related request, report it in our Advanced API forum instead.

StarringLara commented 8 months ago

Pull request #27 is the kind of thing I'm suggesting, but throughout the entire project. This would both help keep the quality of this project high and help users of the SDK to know what types are required and expected. A user's IDE/editor will often show them what types are required or returned from libraries they are using. So the types are helpful to them regardless of whether or not the user is using MyPy. If the user is using MyPy, I don't know enough about it yet to know if having type hints in this project is all that is required for them to get the full benefit of it, but either way, it's still the best path forward so you don't somehow end up providing inaccurate types.

Using type hints for parameters and return values is as much about communication as it is about correctness. If the type of a parameter is not provided, then the user has to either look up the documentation, or look at the implementation, or take their best guess as to what is expected, whereas a type hint would provide absolute and immediate clarity at least about what type to pass in. Another way of looking at it is that not providing a type hint is similar to claiming it will work correctly with anything you pass in, which is usually either not the intended communication or is simply not true.

urischwartz-cb commented 8 months ago

@StarringLara Thank you so much for flagging this! We will look into adding MyPy annotations soon and will update you once that is done. Thanks!

urischwartz-cb commented 8 months ago

@StarringLara Please note that we do have type annotations for all parameters to our public functions. You are right that we have not added type annotations to the return types and will work on adding that soon. Hopefully the existing param types are useful for you regardless!

StarringLara commented 8 months ago

Certainly the existing types are appreciated. I am just getting started with using this SDK, and as I get further with it, I am noticing some type hints in the code, and I do find those very helpful. In a certain sense, it's definitely not an all or nothing thing, and I appreciate any types you are able to provide or add. In another sense, I hope you won't dismiss the concept that the ideal is to have types everywhere, treating it as requiring everything to be typed from then on. Of course, the ideal should not hold you back from making incremental progress now, nor does it make that progress unappreciated. As far as I understand it, the best way to eventually enforce the ideal would be for you to use mypy internally as your own QA tool for this project, and to require it to report no issues in your CI tests, while using it with the setting strict = true, which I think will require all parameters and return values to have a type.

urischwartz-cb commented 8 months ago

@StarringLara we just pushed a new v1.1.3 that includes fully annotated return types for all of our exposed functions. Hopefully that helps in your development. Please let us know if you have any other questions or recommendations for our SDK. Thanks for your feedback and for helping us improve our product!