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

Deprecate app.make_handler() in favor of runners API #2875

Closed
asvetlov opened this issue Mar 25, 2018 · 6 comments
Closed

Deprecate app.make_handler() in favor of runners API #2875

asvetlov opened this issue Mar 25, 2018 · 6 comments
Labels
documentation Improvements or additions to documentation enhancement good first issue Good for newcomers outdated server
Milestone

Comments

@asvetlov
Copy link
Member

Don't change internal logic a lot but add app._make_handler() internal method and reuse it by runners.
More intrusive changes can be done after app.make_handler() removal.
Document app.make_handler() as deprecated, drop the usage from logging.rst and testing.rst.

@asvetlov asvetlov added enhancement good first issue Good for newcomers server documentation Improvements or additions to documentation labels Mar 25, 2018
@asvetlov asvetlov added this to the 3.2 milestone Mar 25, 2018
@asvetlov
Copy link
Member Author

The issue is mostly about documentation update (and a little coding).
A volunteer is welcome!

@xstrengthofonex
Copy link
Contributor

xstrengthofonex commented Apr 12, 2018

I'd like to do this, but would it possible to get some more information about this change. So you want to depreciate the use of app._make_handler() as a "public" api and rename it to app._make_handler()? But the behavior shouldn't be changed right? as the method is still used by AppRunner.

How about the tests inside test_web_app? When I changed the method name to app._handle_request() they kept failing despite changing the method names in the tests.

Also what is the convention for depreciating a method? I couldn't find any example when I looked through the documentation. Is it the .. versionchanged:: 3.2 and then a note about the specific change?

@asvetlov
Copy link
Member Author

The step-by-step instruction is:

  1. Rename .make_handler() to ._make_handler()
  2. Fix tests and code to use app._make_handler()
  3. Add Application.make_handler() back, the method should raise DeprecationWarning like here
    warnings.warn("@streamer is deprecated, use async generators instead",
    DeprecationWarning,
    stacklevel=2)
    and call ._make_handler().
  4. Add .. deprecated:: 3.2 to documentation.
  5. Add xxx.removal file to CHANGES.

@xstrengthofonex
Copy link
Contributor

Thank you for the clarification. I'll get to work

@asvetlov
Copy link
Member Author

Fixed by #2938

@lock
Copy link

lock bot commented Oct 28, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs.
If you feel like there's important points made in this discussion, please include those exceprts into that [new issue].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new

@lock lock bot added the outdated label Oct 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Improvements or additions to documentation enhancement good first issue Good for newcomers outdated server
Projects
None yet
Development

No branches or pull requests

2 participants