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

Fix mypy issues in openapi.py with missing dict and use of typedDict #1034

Closed
wants to merge 3 commits into from

Conversation

dave42w
Copy link
Contributor

@dave42w dave42w commented Nov 17, 2024

Description

This PR fixes mypy issues in openapi.py

Summary

This PR fixes the following

TypedDict

all TypedDict replaced with dict
the python docs don't include any examples of using TypedDict for a variable, only as a class.
mypy 1.13.0 complains about examples like this query_params: Optional[TypedDict], so I have changed them all to be like query_params: Optional[Dict], All tests still pass

MyPy error: Variable "typing.TypedDict" is not valid as a type

This meant changing a few tests from

query_param_annotations = query_params.__annotations__ if query_params is TypedDict else typing.get_type_hints(query_params)

to

query_param_annotations = query_params.__annotations__ if query_params is Dict else typing.get_type_hints(query_params)

While the tests all pass I'm not sure if the logic of typing.get_type_hints still works for Dict vs TypedDict

missing dict type

def get_path_obj variable openapi_path_object

This wasn't given a type which was causing multiple mypy errors:

openapi_path_object = {
...
                openapi_path_object["parameters"].append(
"Sequence[str]" has no attribute "append"
...
            openapi_path_object["requestBody"] = request_body_object
Incompatible types in assignment (expression has type "dict[str, dict[str, dict[str, dict[str, Collection[str]]]]]", target has type "Sequence[str]")
...
        openapi_path_object["responses"] = {"200": {"description": "Successful Response", "content": {response_type: {"schema": response_schema}}}}
Incompatible types in assignment (expression has type "dict[str, dict[str, Collection[str]]]", target has type "Sequence[str]")

all fixed by changing the declaration to:

        openapi_path_object: dict = {

def get_schema_object local variable properties

same as above. Change declaration to

        properties: dict = {

all the similar warnings are gone

my results

These changes mean that mypy 1.13 finds no warnings in openapi.py

PR Checklist

Please ensure that:

  • The PR contains a descriptive title
  • The PR contains a descriptive summary of the changes
  • You build and test your changes before submitting a PR.
  • You have added relevant documentation
  • You have added relevant tests. We prefer integration tests wherever possible

Pre-Commit Instructions:

Copy link

vercel bot commented Nov 17, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
robyn ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 20, 2024 10:46pm

Copy link

codspeed-hq bot commented Nov 17, 2024

CodSpeed Performance Report

Merging #1034 will not alter performance

Comparing dave42w:mypy (989a0ef) with main (3f2e79a)

Summary

✅ 146 untouched benchmarks

@sansyrox
Copy link
Member

Hey @dave42w 👋

What python version are you using?

@dave42w
Copy link
Contributor Author

dave42w commented Nov 17, 2024

Hey @dave42w 👋

What python version are you using?

Python 3.12.7
and
mypy 1.13.0

@dave42w
Copy link
Contributor Author

dave42w commented Nov 20, 2024

@sansyrox Are we ok to merge?

@@ -293,7 +293,7 @@ def get_path_obj(
endpoint_with_path_params_wrapped_in_braces += "/{" + path_param_name + "}"

if query_params:
query_param_annotations = query_params.__annotations__ if query_params is TypedDict else typing.get_type_hints(query_params)
query_param_annotations = query_params.__annotations__ if query_params is Dict else typing.get_type_hints(query_params)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these functions wouldn't make sense on a regular dict.

Comment on lines +239 to +241
query_params: Optional[Dict],
request_body: Optional[Dict],
return_annotation: Optional[Dict],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What error prompted these changes??

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll check tomorrow.
I think the solution (to follow all the docs I can find) would be to have our own class of TypedDict rather than use TypedDict directly.

Copy link
Member

@sansyrox sansyrox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @dave42w 👋

Thank you for the PR. I just have a few follow up questions 😄

@dave42w dave42w closed this Nov 22, 2024
@dave42w dave42w deleted the mypy branch November 22, 2024 23:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants