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

Document that middleware factories are run for every request. #2230

Merged
merged 1 commit into from
Aug 30, 2017

Conversation

butla
Copy link
Contributor

@butla butla commented Aug 29, 2017

I guess the title says it all. Related to #2225

@butla
Copy link
Contributor Author

butla commented Aug 29, 2017

Wait a sec, I should have based that on the released tag... Gotta change the branch.

@codecov-io
Copy link

codecov-io commented Aug 29, 2017

Codecov Report

Merging #2230 into 2.2 will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##              2.2    #2230   +/-   ##
=======================================
  Coverage   97.06%   97.06%           
=======================================
  Files          38       38           
  Lines        7691     7691           
  Branches     1341     1341           
=======================================
  Hits         7465     7465           
  Misses        102      102           
  Partials      124      124

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6d27412...383ef29. Read the comment docs.

docs/web.rst Outdated
@@ -904,6 +904,10 @@ the **next** *middleware factory*. The last *middleware factory* always receives
the :ref:`request handler <aiohttp-web-handler>` selected by the router itself
(by :meth:`UrlDispatcher.resolve`).

.. note::

The chain of factories is run for every handled request.
Copy link
Member

Choose a reason for hiding this comment

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

I think someone could still read this and think it's just reiterating that each middleware is called for all views.

Perhaps we could be more explicit, something like

"Both the outer middleware_factory coroutine and the inner middleware_handler coroutine are called for every request handled."

@butla
Copy link
Contributor Author

butla commented Aug 29, 2017

@samuelcolvin Good idea, changed according to your suggestion.

Copy link
Member

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

LGTM

@samuelcolvin please merge it

@samuelcolvin samuelcolvin merged commit 60f3342 into aio-libs:2.2 Aug 30, 2017
@samuelcolvin
Copy link
Member

done. Thanks @butla.

@asvetlov depending on the outcome of #2225 we might need to add this to master too, do we have somewhere to record that so we don't forget?

@asvetlov
Copy link
Member

I've merged 2.2 into master.

@asvetlov
Copy link
Member

@samuelcolvin in aiohttp we do merging release branches like 2.2 into master after every PR like this.
Well, the rule was never pronounced but I always did it.
aiohttp committers, please do it too.
After switching changenotes to towncrier it is not painful job, we have no merge conflicts in CHANGES.rst anymore.

@samuelcolvin
Copy link
Member

humm, I'm not seeing this commit or the change on master?

@asvetlov
Copy link
Member

Missed to make git push before sending the message.

@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.

@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants