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

fixed type checking errors. #68

Merged
merged 6 commits into from
Nov 22, 2019
Merged

fixed type checking errors. #68

merged 6 commits into from
Nov 22, 2019

Conversation

SKalt
Copy link
Contributor

@SKalt SKalt commented Nov 18, 2019

Resolves #67.

I had to swap @dataclasses for __init__(self, *args) syntax to placate mypy. It isn't ideal, but it was less dumb than other possible workarounds, such as having self.app be a tuple of (app: ASGIApp,).

demonstration of the trouble with @dataclass
from dataclasses import dataclass
from typing import Callable, Dict

Foo = Callable[[Dict[str, str]], int]


def foo(d: Dict[str, str]) -> int:
    return len(list(d.keys()))


@dataclass
class Bar:
    foo: Foo  # mypy thinks the first argument is `self`!

    def run(self, d: Dict[str, str]) -> None:
        print(self.foo(d))
        # Invalid self argument "Bar" to attribute function "foo" with type "Callable[[Dict[str, str]], int]"mypy(error)
        # Too many argumentsmypy(error)


bar = Bar(foo)
bar.run({"a": "1"})  # works, but doesn't pass the type checker


class Baz(object):
    def __init__(self, foo: Foo):
        self.foo = foo  # explicit enough that mypy doesn't think `self.foo` is passed `self`

    def run(self, d: Dict[str, str]) -> None:
        print(self.foo(d))

baz = Baz(foo)
bar.run({"a": "1"})  # works **and** passes type checking

I had to swap @DataClass for __init__(self, args, to, add, to, instance) syntax for mypy.  It isn't ideal, but it was __less__ dumb than other possible workarounds, such as having self.app be a tuple of (app: ASGIApp,).
@SKalt
Copy link
Contributor Author

SKalt commented Nov 18, 2019

@ERM : I can see that travis-ci is firing, but I can't see the test results for this PR. I think you'll need to configure either the github webhooks settings or the travisc-ci run itself.

@jordaneremieff
Copy link
Collaborator

@SKalt thanks for this! These are definitely some fixes I'd like to get in here - very much appreciate the PR. :)

I'll give this a proper review soon, but looks good so far.

@jordaneremieff
Copy link
Collaborator

jordaneremieff commented Nov 20, 2019

@SKalt so I looked into the issue with dataclasses to see if there may be a solution that would allow us to keep using them with mypy. I'm thinking rather than refactoring the class syntax that we could try something like changing the ASGIApp definition to use the callback protocol as suggested in python/mypy#5485 (comment):

# mangum/typing.py

from typing_extensions import Protocol

class ASGIApp(Protocol):
    async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None:
        ...

What do you think? My initial testing with this seems to pass the linter and tests fine, though this definition might require some tweaking.

@SKalt
Copy link
Contributor Author

SKalt commented Nov 20, 2019 via email

@jordaneremieff
Copy link
Collaborator

jordaneremieff commented Nov 21, 2019

@SKalt Great, let's go ahead with the Protocol solution here (and your proposed import statements for 3.8 support) - thanks.

@SKalt
Copy link
Contributor Author

SKalt commented Nov 21, 2019 via email

@jordaneremieff
Copy link
Collaborator

did you happen to do your tests on top of my branch? On Wed, Nov 20, 2019 at 8:35 PM Jordan Eremieff [email protected] wrote:

The only trade-off I see is having to write try: from typing import Protocol # 3.8+ except ImportError: from typing_extensions import Protocol #2.7-3.7 Which I'll say is better than reverting to full-on classes. … <#m_8069571441260918262_> On Wed, Nov 20, 2019, 01:01 Jordan Eremieff @.***> wrote: @SKalt https://github.com/SKalt https://github.com/SKalt so I looked into the issue with dataclasses to see if there may be a solution that would allow us to keep using them with mypy. I'm thinking rather than refactoring the class syntax that we could try something like changing the ASGIApp definition to use the callback protocol https://mypy.readthedocs.io/en/latest/protocols.html#callback-protocols as suggested in python/mypy#5485 <python/mypy#5485> (comment) <python/mypy#5485 (comment) <python/mypy#5485 (comment)>>: # mangum/typing.py class ASGIApp(Protocol): def call( self, scope: Scope, receive: Receive, send: Send ) -> typing.Awaitable[None]: ... What do you think? My initial testing with this seems to pass the linter and tests fine. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#68 <#68>?email_source=notifications&email_token=ACPUNZO6SSGBSGS7P7NPFXTQUTHFNA5CNFSM4JOXGGFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEQ2IEI#issuecomment-555852817>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACPUNZO7D6K2HE7BJHDTKNLQUTHFNANCNFSM4JOXGGFA . Great, let's go ahead with the Protocol solution here (and your proposed import statements for 3.8 support) - thanks. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#68?email_source=notifications&email_token=ACPUNZPKUVFB6DEYZGSOWOLQUXQWBA5CNFSM4JOXGGFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEX7BEI#issuecomment-556789905>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACPUNZLRLAB7V5R4ULZ2JU3QUXQWBANCNFSM4JOXGGFA .

Yes. What I did to test was revert the Mangum adapter class back to a dataclass and included the protocol type in mangum/typing.py on this branch.

@jordaneremieff jordaneremieff merged commit 14a13dc into Kludex:master Nov 22, 2019
@jordaneremieff
Copy link
Collaborator

Thanks!

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.

Pass mypy checks
2 participants