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

support aiohttp-3.8.0 #31

Closed
wants to merge 1 commit into from
Closed

support aiohttp-3.8.0 #31

wants to merge 1 commit into from

Conversation

cmalek
Copy link

@cmalek cmalek commented Nov 2, 2021

This fixes the rename of aiohttp.web_routedef._SimpleHandler to aiohttp.typedefs.Handler

@@ -3,7 +3,7 @@

from aiohttp import web
from aiohttp.web_app import _Middleware
from aiohttp.web_routedef import _SimpleHandler
from aiohttp.typedefs import Handler
Copy link

@kammala kammala Nov 3, 2021

Choose a reason for hiding this comment

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

this will break compatibility with aiohttp < 3.8.0
should we use some kind of conditional imports?

try:
  from aiohttp.typedefs import Handler
except ImportError:
  from aiohttp.web_routedef import _SimpleHandler as Handler

@@ -3,7 +3,7 @@

from aiohttp import web
from aiohttp.web_app import _Middleware
Copy link

Choose a reason for hiding this comment

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

this one was also moved to aiohttp.typedefs, but wasn't released yet.
for future stability, I would recommend to use the same mechanic with conditional import.

Copy link
Author

Choose a reason for hiding this comment

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

Duh, of course, sorry about that. I should have thought about backwards compatibility. I shall make your suggested changes.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I've made the changes, amended the commit and pushed.

Copy link

Choose a reason for hiding this comment

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

looks good, but I am not sure if mypy will be satisfied. did you run make lint against your code?

@cmalek cmalek force-pushed the master branch 2 times, most recently from 55cc99e to eb2a04e Compare November 3, 2021 16:16
This fixes the rename of aiohttp.web_routedef._SimpleHandler to aiohttp.typedefs.Handler while remaining backwards incompatible.   This also fixes the upcoming rename of aiohttp.web_app._Middleware to aiohttp.typedefs.Middleware.
@cmalek
Copy link
Author

cmalek commented Nov 12, 2021

Yes, mypy is not satisfied. make lint says:

> make lint
< linting using black...
All done! ✨ � ✨
22 files would be left unchanged.
> done

< linting using flake8...
> done

< linting using isort...
> done

< linting using mypy...
aiodogstatsd/contrib/aiohttp.py:7: error: Module "aiohttp.typedefs" has no attribute "Middleware"
aiodogstatsd/contrib/aiohttp.py:15: error: Module "aiohttp.web_routedef" has no attribute "_SimpleHandler"
aiodogstatsd/contrib/aiohttp.py:15: error: Name "Handler" already defined (possibly by an import)
Found 3 errors in 1 file (checked 22 source files)
make: *** [lint-mypy] Error 1

I don't know how to fix that since in 3.8.0 aiohttp.typedefs.Middleware certainly does exist, aiohttp.web_routedef._SimpleHandler definitely does not exist, and Handler is not imported twice.

I don't use mypy for linting in our codebases, so what do you suggest?

@kammala
Copy link

kammala commented Nov 15, 2021

well, as far as I see, mypy doesn't recognize this try ... except ImportError pattern.
possible solutions would be:

  • use # type: ignore
try:
  from aiohttp.typedefs import Handler  # type: ignore
except ImportError:
  from aiohttp.web_routedef import _SimpleHandler as Handler  # type: ignore

doesn't make much sense as we use these classes only for typing and ignoring types makes it kinda useless

  • somehow detect aiohttp actual version(via pkg_resources, e.g.) and use if - else conditions.
    this option looks a little bit overcomplicated for type hints and I didn't check if it actually satisfies mypy.
  • do not rely on aiohttp typedefs at all and use just Callable(or something more nested)
Handler = Callable
...

this is the simplest and the most stable solution, but it also kinda compromises idea of type checking.

as I am not a maintainer of this library, I can't make this decision :)
personally, I would go with the latest option(Callable instead of aiohttp internals).

@Gr1N
Copy link
Owner

Gr1N commented Dec 12, 2021

Hi everyone, thanks for the contribution, but decided to go with another solution. Nevertheless, fix already in master.

@Gr1N Gr1N closed this Dec 12, 2021
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