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

Remove asgiref from minimal requirements #1460

Closed
wants to merge 2 commits into from
Closed

Conversation

Kludex
Copy link
Member

@Kludex Kludex commented Apr 26, 2022

This PR removes asgiref from the minimal requirements.

Changes:

  • Leverage from __future__ import annotations to get a small diff - besides the collateral benefits.
  • Move asgiref from minimal_requirements to extra_requirements.

@Kludex Kludex changed the title Remove asgiref from minimal requirements Remove asgiref from minimal requirements Apr 26, 2022
@Kludex Kludex requested review from euri10 April 27, 2022 05:50
@Kludex Kludex modified the milestone: Version 0.18.0 Apr 27, 2022
@Kludex Kludex requested a review from a team April 28, 2022 06:35
@br3ndonland
Copy link
Contributor

In general I think this looks fine, and I'm glad to see Uvicorn entering the __future__.

There's been a lot of discussion about lumping all the optional dependencies into uvicorn[standard] though, and asgiref isn't directly related to the other ones. Do we want to make people install all the optional dependencies just to do type checking? I'm not sure.

Related:

@Kludex
Copy link
Member Author

Kludex commented May 3, 2022

You have a good point. Then, are we against having it as optional but not on the standard extras? 🤔

Copy link
Member

@adriangb adriangb left a comment

Choose a reason for hiding this comment

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

Is this worth it? I understand that Uvicorn wants to be minimal on dependencies, but I'm not sure the internal complexity with TYPE_CHECKING is worth it. I also agree that due to how the extras are currently specified it's problematic to ask users to install a bunch of dependencies just to get type checking, and in fact that is contrary to the goal of not forcing dependencies on users (granted, users could look into the extra and cherry pick what they want but that's quite a bit to ask and defeats the point of extras in the first place). Somewhat tangential, but I think this would be a good fit for a default-extras if it existed.

Comment on lines +5 to +11
if typing.TYPE_CHECKING:
from asgiref.typing import (
ASGI2Application,
ASGIReceiveCallable,
ASGISendCallable,
Scope,
)
Copy link
Member

Choose a reason for hiding this comment

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

This is quite a bit of boilerplate that we'll have to have in every module

@@ -1,12 +1,16 @@
from typing import List, Union
from __future__ import annotations
Copy link
Member

Choose a reason for hiding this comment

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

Given that this __future__ is in somewhat of a limbo, I would be reluctant to merge it.

@Kludex
Copy link
Member Author

Kludex commented May 3, 2022

Is this worth it? I understand that Uvicorn wants to be minimal on dependencies, but I'm not sure the internal complexity with TYPE_CHECKING is worth it.

This is more for django-channel users than to be minimal (but also): #1305.

@Kludex
Copy link
Member Author

Kludex commented Jun 20, 2022

@Kludex Kludex closed this Jun 20, 2022
@Kludex Kludex deleted the remove-asgiref branch October 2, 2022 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants