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

Fixed misses in helpers #1521

Closed
wants to merge 1 commit into from
Closed

Fixed misses in helpers #1521

wants to merge 1 commit into from

Conversation

AraHaan
Copy link
Contributor

@AraHaan AraHaan commented Dec 29, 2016

This commit fixes all 4 of the misses in helpers.py.

What do these changes do?

Fix the Misses in helpers.py

Are there changes in behavior for the user?

No changes in behavior at all.

Related issue number

None

Checklist

  • Unit tests for the changes exist
  • I think the code is well written

@codecov-io
Copy link

codecov-io commented Dec 29, 2016

Current coverage is 98.94% (diff: 100%)

Merging #1521 into master will increase coverage by 0.05%

@@             master      #1521   diff @@
==========================================
  Files            30         30          
  Lines          6990       6984     -6   
  Methods           0          0          
  Messages          0          0          
  Branches       1163       1162     -1   
==========================================
- Hits           6912       6910     -2   
+ Misses           39         35     -4   
  Partials         39         39          

Powered by Codecov. Last update 8469b6d...ce9f016

@AraHaan AraHaan changed the title Fix Misses in helpers Fix some misses in helpers Dec 29, 2016
@AraHaan AraHaan changed the title Fix some misses in helpers Fixed some misses in helpers Dec 29, 2016
@AraHaan AraHaan changed the title Fixed some misses in helpers Fixed misses in helpers Dec 29, 2016
@AraHaan
Copy link
Contributor Author

AraHaan commented Dec 29, 2016

@fafhrd91 What you think of this PR? Also I noticed there was a missing 0 in the version so I fixed that as well in this PR, btw you could also look in #1518 as well to. It also contains my entry in CONTRIBUTORS.txt

@fafhrd91
Copy link
Member

i don't think it worth to create new module just for import checks.
also it seems nothing uses check_loop

@AraHaan
Copy link
Contributor Author

AraHaan commented Dec 29, 2016

So, Check loop can be safely removed. I see.

@AraHaan
Copy link
Contributor Author

AraHaan commented Dec 30, 2016

Ok, then check_loop has now been removed completely since it is not getting used anyway so yeah.

@fafhrd91
Copy link
Member

I leave this PR to @asvetlov
He is active maintainer

@AraHaan
Copy link
Contributor Author

AraHaan commented Dec 30, 2016

Ok, found a way to help get what we need. Using getattr for if asyncio.ensure_future does not exist it returns asyncio.async and use 1 line.

ss 2016-12-30 at 04 02 04

This commit fixes all 4 of the misses in this file.
@AraHaan
Copy link
Contributor Author

AraHaan commented Dec 30, 2016

Looks like I just had to monkey patch 1 of the tests as sometimes it would fail at web.run_app hmm but is more often then not rare. So, when it happens should make it instead of the pass that I am doing right now to make it xfail

@asvetlov
Copy link
Member

I've dropped check_loop and fixed version number. 39b8568
ensure_future is covered by running on different python versions.

@asvetlov asvetlov closed this Dec 31, 2016
@AraHaan AraHaan deleted the helpers_patch branch December 31, 2016 01:41
@lock
Copy link

lock bot commented Oct 29, 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 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 29, 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