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

chore: reduce number of mypy warnings #1023

Merged
merged 31 commits into from
Nov 15, 2024
Merged

chore: reduce number of mypy warnings #1023

merged 31 commits into from
Nov 15, 2024

Conversation

dave42w
Copy link
Contributor

@dave42w dave42w commented Nov 11, 2024

Description

This PR cleans up some mypy warnings in

  • init.py the ones for SubRouter fix bugs
  • authenticate.py
  • argument_parser.py
  • dependency_injection.py
  • reloader.py
  • openapi.py
  • processpool.py

Summary

This PR only contains fixes to mypy warnings (using latest mypy 1.13.0)

NB The fixes for SubRouter are significant bug as auth_required was missing. I've added tests to confirm subroute get authentication but they are failing with AuthenticationNotConfiguredError: Authentication is not configured. Use app.configure_authentication() to configure it. see #1026

NB there are no integration tests for authentication except for HTTP GET, we need them for POST, PUT etc

init.py

I've added an issue to nestd for types see sparckles/nestd#6

mutltiprocess already has an issue raised for typing. I've used the workaround in that thread.

Starting point for init.py (All complete apart from nestd)

robyn/init.py:8: error: Cannot find implementation or library stub for module named "multiprocess" [import-not-found]
robyn/init.py:9: error: Cannot find implementation or library stub for module named "nestd" [import-not-found]
robyn/init.py:8: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
robyn/init.py:45: error: Incompatible default for argument "openapi_file_path" (default has type "None", argument has type "str")
robyn/init.py:82: error: Need type annotation for "event_handlers" (hint: "event_handlers: dict[, ] = ...") [var-annotated]
robyn/init.py:599: error: Signature of "get" incompatible with supertype "Robyn" [override]
robyn/init.py:599: note: Superclass:
robyn/init.py:599: note: def get(self, endpoint: str, const: bool = ..., auth_required: bool = ..., openapi_name: str = ..., openapi_tags: list[str] = ...) -> Any
robyn/init.py:599: note: Subclass:
robyn/init.py:599: note: def get(self, endpoint: str, const: bool = ..., openapi_name: str = ..., openapi_tags: list[str] = ...) -> Any
robyn/init.py:602: error: Signature of "post" incompatible with supertype "Robyn" [override]
robyn/init.py:602: note: Superclass:
robyn/init.py:602: note: def post(self, endpoint: str, auth_required: bool = ..., openapi_name: str = ..., openapi_tags: list[str] = ...) -> Any
robyn/init.py:602: note: Subclass:
robyn/init.py:602: note: def post(self, endpoint: str, openapi_name: str = ..., openapi_tags: list[str] = ...) -> Any
robyn/init.py:605: error: Signature of "put" incompatible with supertype "Robyn" [override]
robyn/init.py:605: note: Superclass:
robyn/init.py:605: note: def put(self, endpoint: str, auth_required: bool = ..., openapi_name: str = ..., openapi_tags: list[str] = ...) -> Any
robyn/init.py:605: note: Subclass:
robyn/init.py:605: note: def put(self, endpoint: str, openapi_name: str = ..., openapi_tags: list[str] = ...) -> Any
robyn/init.py:608: error: Signature of "delete" incompatible with supertype "Robyn" [override]
robyn/init.py:608: note: Superclass:
robyn/init.py:608: note: def delete(self, endpoint: str, auth_required: bool = ..., openapi_name: str = ..., openapi_tags: list[str] = ...) -> Any
robyn/init.py:608: note: Subclass:
robyn/init.py:608: note: def delete(self, endpoint: str, openapi_name: str = ..., openapi_tags: list[str] = ...) -> Any
robyn/init.py:611: error: Signature of "patch" incompatible with supertype "Robyn" [override]
robyn/init.py:611: note: Superclass:
robyn/init.py:611: note: def patch(self, endpoint: str, auth_required: bool = ..., openapi_name: str = ..., openapi_tags: list[str] = ...) -> Any
robyn/init.py:611: note: Subclass:
robyn/init.py:611: note: def patch(self, endpoint: str, openapi_name: str = ..., openapi_tags: list[str] = ...) -> Any
robyn/init.py:614: error: Signature of "head" incompatible with supertype "Robyn" [override]
robyn/init.py:614: note: Superclass:
robyn/init.py:614: note: def head(self, endpoint: str, auth_required: bool = ..., openapi_name: str = ..., openapi_tags: list[str] = ...) -> Any
robyn/init.py:614: note: Subclass:
robyn/init.py:614: note: def head(self, endpoint: str, openapi_name: str = ..., openapi_tags: list[str] = ...) -> Any
robyn/init.py:617: error: Signature of "trace" incompatible with supertype "Robyn" [override]
robyn/init.py:617: note: Superclass:
robyn/init.py:617: note: def trace(self, endpoint: str, auth_required: bool = ..., openapi_name: str = ..., openapi_tags: list[str] = ...) -> Any
robyn/init.py:617: note: Subclass:
robyn/init.py:617: note: def trace(self, endpoint: str, openapi_name: str = ..., openapi_tags: list[str] = ...) -> Any
robyn/init.py:620: error: Signature of "options" incompatible with supertype "Robyn" [override]
robyn/init.py:620: note: Superclass:
robyn/init.py:620: note: def options(self, endpoint: str, auth_required: bool = ..., openapi_name: str = ..., openapi_tags: list[str] = ...) -> Any
robyn/init.py:620: note: Subclass:
robyn/init.py:620: note: def options(self, endpoint: str, openapi_name: str = ..., openapi_tags: list[str] = ...) -> Any

authenticate.py line 84

Fixed from if "authorization" in request.headers: to if request.headers.contains("authorization"):

argument_parser.py line 106

Fixed from

            self.processes = self.processes or ((os.cpu_count() * 2) + 1) or 1

to

            cpu_count: int = os.cpu_count() or 1
            self.processes = self.processes or ((cpu_count * 2) + 1) or 1

because mypy warned that os.cpu_count() can be None

dependency_injection

change from

class DependencyMap:
    def __init__(self):

to

class DependencyMap:
    def __init__(self) -> None:

to get type checking in the ini method

reloader.py

Added List to self.built_rust_binaries: List = []

openapi.py

Fixed line 242 from ) -> (str, dict): to tuple[str, dict]:
Fixed spelling of Definition line 248
Fixed line 331 from response_schema = {} to response_schema: dict = {}
Fixed line 419 from def get_openapi_docs_page(self) -> FileResponse: to def get_openapi_docs_page(self) -> Response: which meant removing FileResponse from imports

Some still left

robyn/openapi.py:239: error: Variable "typing.TypedDict" is not valid as a type  [valid-type]
robyn/openapi.py:239: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
robyn/openapi.py:240: error: Variable "typing.TypedDict" is not valid as a type  [valid-type]
robyn/openapi.py:240: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
robyn/openapi.py:241: error: Variable "typing.TypedDict" is not valid as a type  [valid-type]
robyn/openapi.py:241: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
robyn/openapi.py:285: error: "Sequence[str]" has no attribute "append"  [attr-defined]
robyn/openapi.py:338: error: Incompatible types in assignment (expression has type "dict[str, dict[str, Collection[str]]]", target has type "Sequence[str]")  [assignment]
robyn/openapi.py:342: error: Variable "typing.TypedDict" is not valid as a type  [valid-type]
robyn/openapi.py:342: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
robyn/openapi.py:394: error: Incompatible types in assignment (expression has type "list[dict[str, str]]", target has type "str")  [assignment]
robyn/openapi.py:400: error: Incompatible types in assignment (expression has type "dict[Never, Never]", target has type "str")  [assignment]
robyn/openapi.py:403: error: Unsupported target for indexed assignment ("str")  [index]

processpool.py

Fixed from multiprocess import Process same as in init.py
Fixed line 88 from process_pool = [] to process_pool: List = []

…h" (default has type "None", argument has type "str")"
Copy link

vercel bot commented Nov 11, 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 15, 2024 6:03pm

@dave42w dave42w changed the title reduce number of mypy warnings reduce number of mypy warnings in __init__.py Nov 11, 2024
robyn/__init__.py Outdated Show resolved Hide resolved
@dave42w
Copy link
Contributor Author

dave42w commented Nov 11, 2024

I've also noticed that the only Http method with integration tests for authentication is GET. There are no tests for POST, PUT etc

…edError: Authentication is not configured. Use app.configure_authentication() to configure it.
…edError: Authentication is not configured. Use app.configure_authentication() to configure it.
…edError: Authentication is not configured. Use app.configure_authentication() to configure it.
@dave42w
Copy link
Contributor Author

dave42w commented Nov 11, 2024

I've commited failing tests for subrouter authentication. Somehow the authentication isn't being configured for subroutes?

@dave42w dave42w changed the title reduce number of mypy warnings in __init__.py reduce number of mypy warnings Nov 11, 2024
@dave42w
Copy link
Contributor Author

dave42w commented Nov 11, 2024

NB
Both openapi.py and processpool.api still have some significant mypy issues

robyn/openapi.py Outdated Show resolved Hide resolved
robyn/openapi.py Outdated Show resolved Hide resolved
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.

LGTM! Thank you @dave42w 😄

@sansyrox sansyrox changed the title reduce number of mypy warnings chore: reduce number of mypy warnings Nov 14, 2024
@sansyrox
Copy link
Member

Hey @dave42w 👋

looks like the ci is failing

@dave42w
Copy link
Contributor Author

dave42w commented Nov 14, 2024

That will be the subroute authentication tests I added that don't pass because authentication doesn't seem to be fully setup for subroutes

@dave42w
Copy link
Contributor Author

dave42w commented Nov 14, 2024

We could just not include the extra tests at this point.

@dave42w
Copy link
Contributor Author

dave42w commented Nov 15, 2024

@sansyrox I've removed all those api changes around the authorisation (that were trying to gain consistency with the SubRouter superclass).

I figure that is a bigger job that needs to be separate and have tests.

So now it is just the mypy cleanups that you have already approved.

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.

LGTM! Thank you @dave42w

Copy link

codspeed-hq bot commented Nov 15, 2024

CodSpeed Performance Report

Merging #1023 will not alter performance

Comparing dave42w:my-py (cc7ee04) with main (b3750fe)

Summary

✅ 146 untouched benchmarks

@sansyrox sansyrox merged commit a94443d into sparckles:main Nov 15, 2024
60 checks passed
@dave42w dave42w deleted the my-py branch November 16, 2024 00:51
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