Skip to content
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.

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

Support for type hints #105

Closed
wthueb opened this issue May 24, 2022 · 11 comments
Closed

Support for type hints #105

wthueb opened this issue May 24, 2022 · 11 comments
Assignees
Labels
type/feature New feature or request

Comments

@wthueb
Copy link
Contributor

wthueb commented May 24, 2022

The lack of type hinting is a pain for my development workflow, as tools like mypy outright refuse to analyze pyarr code, and even Pylance in VS Code doesn't seem to infer the types properly most of the time.

It should be fairly simple to add types, as most of the API returns dict[str, Any] or list[dict[str, Any]]. It's an implementation decision on whether or not it should be done using stubs or actually implementing it in the codebase.

I can begin working on this if I can find some time, but depending on how far we've gotten on switching to using asyncio, it might be worth waiting until after that's done just so merging won't be a pain.

@Archmonger
Copy link
Contributor

asyncio support is still very rudimentary. I can handle merging if you want to push out a separate PR for type hints

@Archmonger
Copy link
Contributor

Archmonger commented May 24, 2022

Feel free to steal the type hints directly from #93 for your PR.

I only have return types in that PR though, so you'll need to do the args/kwargs.

@marksie1988 marksie1988 self-assigned this May 24, 2022
@github-actions
Copy link

Branch 105-support_for_type_hints created!

@marksie1988
Copy link
Collaborator

I can pick this up as ive got some time spare. I will use use union instead of pipe to keep it compatible with 3.9 unless you see any issues with doing that?

@wthueb
Copy link
Contributor Author

wthueb commented May 24, 2022

Using Unions instead of pipes is fine with me. May as well keep 3.9 support, I don't think that many people have switched over to 3.10 yet.

@marksie1988
Copy link
Collaborator

Yea that was my thinking, can always swap it at a later date when 3.9 is not as common

@marksie1988
Copy link
Collaborator

Ok so I have added all the types however there is an issue that mypy doesnt like when you use a Union of types with a method that then specifies a specific return.

e.g. the request_handler returns multiple different types Union[list[dict], dict, Response] however the methods from say sonarr.py return either list[dict], dict or Response. this causes mypy to error as its expecting the specifc return type from the request handler rather than the union.

There are two possible solutions:

  1. Add assert isinstance(respones, dict) to each of the methods, allows us to keep the return values from the API more flexible
  2. Write out typed dicts for each of the possible responses and specify it on each of the return types. This gives the most strict typing but also means if the Arr APIs are updated, we could have missing fields or fields that have been removed and an update would be required to resolve this.

@Archmonger interested to see what you think would be the best approach as I've not worked with this issue before. I think both have pros and cons but unsure of what is best.

@Archmonger
Copy link
Contributor

Archmonger commented May 26, 2022

I didn't figure out a good solution for this as well. I ended up just specifying Any as one of the return types just to avoid the linting error.

@wthueb
Copy link
Contributor Author

wthueb commented May 26, 2022

Using TypedDict seems a little excessive. I see no downside to using assert isinstance(...) personally.

@marksie1988
Copy link
Collaborator

Yea I'm thinking assert is going to be best, means updating all the methods but will provide the closest accuracy I think.

I'll give it a try and see how it goes.

@wthueb
Copy link
Contributor Author

wthueb commented May 26, 2022

Should probably specify dict[str, Any] as opposed to a dict. Would prevent trying to get ret[0] for example (expecting a list to be returned instead of a dict).

@marksie1988 marksie1988 mentioned this issue May 27, 2022
10 tasks
@marksie1988 marksie1988 added the type/feature New feature or request label Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/feature New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants