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

App cleanup context #2747

Merged
merged 17 commits into from
Mar 11, 2018
Merged

App cleanup context #2747

merged 17 commits into from
Mar 11, 2018

Conversation

asvetlov
Copy link
Member

A fix for #2092

@asvetlov asvetlov added this to the 3.1 milestone Feb 17, 2018
@codecov-io
Copy link

codecov-io commented Feb 17, 2018

Codecov Report

Merging #2747 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2747      +/-   ##
==========================================
+ Coverage   97.98%   97.99%   +<.01%     
==========================================
  Files          39       39              
  Lines        7391     7423      +32     
  Branches     1299     1304       +5     
==========================================
+ Hits         7242     7274      +32     
  Misses         47       47              
  Partials      102      102
Impacted Files Coverage Δ
aiohttp/web.py 98.79% <100%> (ø) ⬆️
aiohttp/web_app.py 99.08% <100%> (+0.15%) ⬆️

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 185ab31...1918663. Read the comment docs.

@asvetlov
Copy link
Member Author

The feature is done (except documentation).
The only questionable thing is the new signal name.
Now it is app.on_cleanup_ctx.
Suggestions?

@asvetlov
Copy link
Member Author

@aio-libs/aiohttp-committers please take a look.
If nobody will invite a better name than app.on_cleanup_ctx I'll merge the PR after adding a documentation for the feature.

@kxepal
Copy link
Member

kxepal commented Feb 25, 2018

I don't see any app.on_cleanup_ctx around in this PR, but app.cleanup_ctx fits nice.

@asvetlov
Copy link
Member Author

Sorry, it is definitely app.cleanup_ctx property.

@kxepal
Copy link
Member

kxepal commented Feb 25, 2018

It has a good name since it refrect exactly what it reference to - CleanupContext. It's hard to figure something better for me. Why do you question this name? Do you have any different ideas about?

@asvetlov
Copy link
Member Author

No other ideas actually.
But I was not sure, maybe somebody can suggest something better?

@code-cro
Copy link
Contributor

code-cro commented Mar 7, 2018

I suggest full name cleanup_context to be more clear for the reader.

@asvetlov
Copy link
Member Author

asvetlov commented Mar 7, 2018

Thanks for sharing opinion. Ctx is pretty common abbreviation.
Python uses such shortcuts everywhere in stdlib.
cleanup_context is really long name

@asvetlov asvetlov merged commit 374f173 into master Mar 11, 2018
@asvetlov asvetlov deleted the app-cleanup-ctx branch March 11, 2018 19:05