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

Added depreciated warning in filter warning #3198

Closed
wants to merge 3 commits into from

Conversation

shivamdurgbuns
Copy link

@shivamdurgbuns shivamdurgbuns commented Jul 30, 2022

Signed-off-by: Shivam Durgbuns shivamdurgbuns@gmail.com
fixes: #3176

Description

Checklist - did you ...

  • Add a CHANGELOG entry if necessary?
  • Add / update tests if necessary?
  • Add new / update outdated documentation?

@github-actions
Copy link

diff-shades reports zero changes comparing this PR (a514fa2) to main (eaa0489).


What is this? | Workflow run | diff-shades documentation

@ichard26 ichard26 added skip news Pull requests that don't need a changelog entry. C: maintenance Related to project maintenance, e.g. CI, testing, policy changes, releases labels Aug 1, 2022
Copy link
Collaborator

@ichard26 ichard26 left a comment

Choose a reason for hiding this comment

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

There is no mitigation here, it's simply ignoring the issue. We might want to also try except this decorator though similar to #2974 so when aiohttp does remove this decorator, nothing breaks :)

Thoughts @graingert ?

pyproject.toml Outdated Show resolved Hide resolved
@graingert
Copy link
Contributor

There is no mitigation here, it's simply ignoring the issue. We might want to also try except this decorator though similar to #2974 so when aiohttp does remove this decorator, nothing breaks :)

Thoughts @graingert ?

Yeah you need to mitigate this with a try/catch ImportError

pyproject.toml Outdated
@@ -50,6 +50,8 @@ markers = [
xfail_strict = true
filterwarnings = [
"error",
# this ignore can be removed when @middleware is removed from the codebase.
Copy link
Contributor

Choose a reason for hiding this comment

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

My intention was that you'd apply the mitigation rather than change the comment ;)

Copy link
Author

Choose a reason for hiding this comment

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

@graingert I am not getting it. You want me to raise Import Error exception on catching Depreciation Warning?

Copy link
Contributor

@graingert graingert Aug 13, 2022

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

@graingert can you review now?

@JelleZijlstra
Copy link
Collaborator

Sounds like we should not do this and instead actually fix the warning.

Signed-off-by: Shivam Durgbuns <shivamdurgbuns@gmail.com>
@shivamdurgbuns
Copy link
Author

Sounds like we should not do this and instead actually fix the warning.

Has the fix been made? Or should I add an exception for this?

Signed-off-by: Shivam Durgbuns <shivamdurgbuns@gmail.com>
@ichard26
Copy link
Collaborator

Sorry @shivamdurgbuns, but looks like we haven't been clear enough on what needs to be done. We want to mitigate the deprecation of @middleware in aiohttp 4.x by adding a try/except in src/blackd/middlewares.py that simply does middleware = lambda x: x if aiohttp.web_middlewares.middleware doesn't exist. After that then it's safe to ignore the deprecation warning in pyproject.toml.

diff --git a/pyproject.toml b/pyproject.toml
index 813e86b..849891f 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -105,6 +105,9 @@ filterwarnings = [
     # this is mitigated by a try/catch in https://github.com/psf/black/pull/2974/
     # this ignore can be removed when support for aiohttp 3.7 is dropped.
     '''ignore:Decorator `@unittest_run_loop` is no longer needed in aiohttp 3\.8\+:DeprecationWarning''',
+    # this is mitigated by a try/catch in https://github.com/psf/black/pull/3198/
+    # this ignore can be removed when support for aiohttp 3.x is dropped.
+    '''ignore:Middleware decorator is deprecated since 4\.0 and its behaviour is default, you can simply remove this decorator:DeprecationWarning''',
     # this is mitigated by https://github.com/python/cpython/issues/79071 in python 3.8+
     # this ignore can be removed when support for 3.7 is dropped.
     '''ignore:Bare functions are deprecated, use async ones:DeprecationWarning''',
diff --git a/src/blackd/middlewares.py b/src/blackd/middlewares.py
index 7abde52..2be42b0 100644
--- a/src/blackd/middlewares.py
+++ b/src/blackd/middlewares.py
@@ -1,9 +1,19 @@
-from typing import Awaitable, Callable, Iterable
+from typing import TYPE_CHECKING, Awaitable, Callable, Iterable, TypeVar
 
-from aiohttp.web_middlewares import middleware
 from aiohttp.web_request import Request
 from aiohttp.web_response import StreamResponse
 
+try:
+    from aiohttp.web_middlewares import middleware
+except ImportError:
+    # @middleware is deprecated and its behaviour is the default since aiohttp 4.0 so
+    # if it doesn't exist anymore, just define a no-op decorator for compatibility.
+    middleware = lambda x: x
+
+if TYPE_CHECKING:
+    F = TypeVar("F", bound=Callable[..., Any])
+    middleware: Callable[[F], F] = lambda x: x
+
 Handler = Callable[[Request], Awaitable[StreamResponse]]

Here are roughly the changes we'd like, although this is mostly untested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: maintenance Related to project maintenance, e.g. CI, testing, policy changes, releases skip news Pull requests that don't need a changelog entry.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upcoming aiohttp 4.0 is breaking our test suite with a new deprecation warning
4 participants