Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Use of asgiref typing #1217

Closed
2 tasks done
Kludex opened this issue Jun 25, 2021 · 7 comments
Closed
2 tasks done

Use of asgiref typing #1217

Kludex opened this issue Jun 25, 2021 · 7 comments
Labels
typing Type annotations or mypy issues

Comments

@Kludex
Copy link
Member

Kludex commented Jun 25, 2021

Checklist

  • There are no similar issues or pull requests for this yet.
  • @JayH5 and @Kludex are positive about this idea, but it was not discussed on our Gitter channel.

Is your feature related to a problem? Please describe.

On uvicorn we started a mypy compliance goal since some time ago, and we're about to accomplish it. On this road, we decided to use asgiref typing to annotate ASGI related types.

What I propose is that we do the same for Starlette.

Describe the solution you would like.

Remove the types.py module, and annotate everything that is needed with asgiref.typing types.

There's no matching type to Message, as discussed on #1206 . So even if it would be more pleasant to our users, unfortunately it will be more troublesome to follow that path.

Describe alternatives you considered

Use Message as Union[ASGISendCallable, ASGIReceiveCallable] and solve the mypy issues. It will be more pleasant to ours users.

@JayH5 JayH5 added the typing Type annotations or mypy issues label Aug 16, 2021
@Kludex
Copy link
Member Author

Kludex commented Dec 11, 2021

I guess the first step here would be to replace the Message type by two different message types, one for receive and another one for send.

@aminalaee
Copy link
Member

@Kludex if we can maybe split this into multiple part, I can help with it too.

@Kludex
Copy link
Member Author

Kludex commented Dec 11, 2021

The problem is that I'm not 100% sure, but I think what makes the issue hard to solve is the Message type. You'll see the mypy errors if you experiment some changes locally.

That being said... I believe the approach would be:

  1. Split Message into ASGISendEvent and ASGIReceiveEvent - naming matching asgiref.
  2. Rename the other types to asgiref (just renaming, not introduce the package yet).
  3. Remove the types definition and import from asgiref - this should be straightforward based on step 2.

@br3ndonland
Copy link

The problem is that I'm not 100% sure, but I think what makes the issue hard to solve is the Message type. You'll see the mypy errors if you experiment some changes locally.

That being said... I believe the approach would be:

  1. Split Message into ASGISendEvent and ASGIReceiveEvent - naming matching asgiref.
  2. Rename the other types to asgiref (just renaming, not introduce the package yet).
  3. Remove the types definition and import from asgiref - this should be straightforward based on step 2.

I helped with type-annotating Uvicorn, and I'm happy to help here as well.

I've already made some good progress, and I'm on step 3 now. I need to correct a few type errors, now that we're importing from the more specific asgiref types.

Should have a PR up shortly.

@Kludex
Copy link
Member Author

Kludex commented Dec 15, 2021

@br3ndonland Cool! I'll be happy to review it.

Just take in consideration that the steps mentioned were also a way to minimize the diff and make the review process easier. 🙏

@br3ndonland
Copy link

Just take in consideration that the steps mentioned were also a way to minimize the diff and make the review process easier. 🙏

Totally. I know these refactorings can create large diffs. It can be a lot to review. 😫

I'm trying to keep my commits organized, so you can easily see the steps taken, and walk through commit-by-commit. We can also break it up into multiple PRs if needed.

@Kludex
Copy link
Member Author

Kludex commented Dec 15, 2021

Perfect! Thanks @br3ndonland !

@encode encode locked and limited conversation to collaborators May 8, 2022
@Kludex Kludex converted this issue into discussion #1629 May 8, 2022

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
typing Type annotations or mypy issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants