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

Add type annotations to asgiref functions #429

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

davitacols
Copy link

@davitacols davitacols commented Dec 23, 2023

Changes Made

  • Improved clarity and readability in the is_double_callable function.
  • Enhanced consistency and readability in the double_to_single_callable and guarantee_single_callable functions.

Why

Refactored the code to enhance readability and maintainability. The changes aim to improve understanding and make the codebase more consistent.

Checklist

  • Code adheres to project coding standards.
  • Documentation updated
  • All tests passed locally
  • Pylint 9.28/10

Notes for Reviewers

Please review for clarity and consistency. No functional changes were introduced; the focus is on code organization and readability.

@andrewgodwin
Copy link
Member

Looks like you've got some work to do to make the tests and the linter happy! I'd recommend running them locally; GitHub won't run them for you on every commit because you're a first-time contributor.

@davitacols
Copy link
Author

all 3 tests passed locally

@davitacols
Copy link
Author

Screenshot (66)
tests result

@andrewgodwin
Copy link
Member

Unfortunately the tests passing locally isn't enough - we have a very particular CI setup because asgiref is incredibly hard to test correctly. If you're not already running it, I might recommend running the tests via tox as it will ensure that you have all the right Python versions and fresh virtualenvs for each?

@davitacols
Copy link
Author

Right, there is no problem. I will go ahead with tox

@davitacols
Copy link
Author

mypy keeps failing. rest of the tests looks good.
Screenshot (67)

@andrewgodwin
Copy link
Member

Unfortunately mypy is exactly what you need to satisfy; there's a reason some of this typing hasn't been done - it's very complex to type correctly, as you can see!

@davitacols
Copy link
Author

Unfortunately mypy is exactly what you need to satisfy; there's a reason some of this typing hasn't been done - it's very complex to type correctly, as you can see!

Will continue working on it till it passes the test.

@davitacols
Copy link
Author

import inspect
from typing import Any, Awaitable, Callable

from .sync import iscoroutinefunction


def is_double_callable(application: Any) -> bool:
    """
    Tests to see if an application is a legacy-style (double-callable) application.
    """
    if getattr(application, "_asgi_single_callable", False):
        return False
    if getattr(application, "_asgi_double_callable", False):
        return True
    if inspect.isclass(application):
        return True
    if hasattr(application, "__call__"):
        if iscoroutinefunction(application.__call__):
            return False
    return not iscoroutinefunction(application)


async def double_to_single_callable(application: Any) -> Callable[..., Awaitable[None]]:
    """
    Transforms a double-callable ASGI application into a single-callable one.
    """

    async def new_application(
        scope: Any,
        receive: Callable[..., Awaitable[Any]],
        send: Callable[[Any], Awaitable[None]],
    ) -> None:

        instance = application(scope)
        await instance(receive, send)

    return new_application


def guarantee_single_callable(application: Any) -> Any:
    """
    Takes either a single- or double-callable application and always returns it
    in single-callable style. Use this to add backward compatibility for ASGI
    2.0 applications to your server/test harness/etc.
    """
    if is_double_callable(application):
        application = double_to_single_callable(application)
    return application

This tends to pass mypy test but tox finds some faults in the test_compatibility.py

@tynor
Copy link

tynor commented Jan 13, 2024

I am also interested in getting type annotations added to asgiref.compatibility. :) I have a branch in my fork with working type annotations. I have not run it against the full tox matrix, but it does pass mypy type checking (on Python 3.11) without using Any anywhere (pun not intended).

I'm not experienced enough with Github to know how best to contribute to a PR already in progress (should I just open another one?) but the code is there if @davitacols is interested in integrating it into this PR.

@davitacols
Copy link
Author

I am also interested in getting type annotations added to asgiref.compatibility. :) I have a branch in my fork with working type annotations. I have not run it against the full tox matrix, but it does pass mypy type checking (on Python 3.11) without using Any anywhere (pun not intended).

I'm not experienced enough with Github to know how best to contribute to a PR already in progress (should I just open another one?) but the code is there if @davitacols is interested in integrating it into this PR.

I am so glad to hear about such a progress. Moreover, you can post the compatibilty.py code here in the comment or you can make a fresh PR to my original PR.

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.

3 participants