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

Add separate non-blocking start_app corountine #5391

Closed
wants to merge 3 commits into from

Conversation

Bluenix2
Copy link

@Bluenix2 Bluenix2 commented Jan 9, 2021

What do these changes do?

This will add a separate start corountine allowing the user to use a similar half-performant function as run_app but if they wish to do the keepalive sleeps themselves.

Are there changes in behavior for the user?

This has 0 breaking changes, unless someone depends on the previous _run_app function which was renamed to stat_app and had the keepalive code moved.

Related issue number

This should close #5386, which I opened confused about the pre-commit hooks failing.

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."
  • Unit tests for the changes exist

If there is for run_app then I guess? I did not add any new unit tests for the new start_app function.

The format is <Name> <Surname>.

I do not want to reveal my real name, looking at the document it doesn't seem as if any aliases are there so I am not gonna add myself there. Which is completely fine by me.

Please keep alphabetical order, the file is sorted by names.

Not possible considering run_app is dependent on start_app which means an exception to that rule is made here.

@Bluenix2 Bluenix2 requested a review from asvetlov as a code owner January 9, 2021 23:57
@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Jan 9, 2021
@codecov
Copy link

codecov bot commented Jan 16, 2021

Codecov Report

Merging #5391 (e06168f) into master (9b1792b) will decrease coverage by 0.02%.
The diff coverage is 90.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5391      +/-   ##
==========================================
- Coverage   97.16%   97.14%   -0.03%     
==========================================
  Files          41       41              
  Lines        8746     8752       +6     
  Branches     1402     1402              
==========================================
+ Hits         8498     8502       +4     
- Misses        129      130       +1     
- Partials      119      120       +1     
Flag Coverage Δ
unit 97.04% <90.47%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
aiohttp/web.py 97.63% <90.47%> (-1.51%) ⬇️
aiohttp/web_protocol.py 86.12% <0.00%> (-0.29%) ⬇️
aiohttp/client_reqrep.py 97.52% <0.00%> (+0.14%) ⬆️

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 9b1792b...e06168f. Read the comment docs.

@Bluenix2
Copy link
Author

Okay, I finally found a nice implementation which passes all unit tests. See #5389 as to why some checks get canceled.

Regarding codecov I am not sure, the code it complains about is just code I simply moved? I also don't have any experience writing any new unit tests if that is needed.

@Bluenix2 Bluenix2 closed this May 15, 2021
@Rixxan
Copy link

Rixxan commented Mar 27, 2022

I see this PR was closed without being merged - was that intentional and this PR is no longer in development or did this simply slip through the cracks?

@Bluenix2
Copy link
Author

I closed this because I was fighting the tests - not exactly sure how I would structure the code to make them pass - and later on re-structured my code to no longer need the added function. I intentionally closed this PR and I am not working on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Separated start corountine
2 participants