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 ClientSession initialization warning #1518

Closed
wants to merge 2 commits into from
Closed

Fixed ClientSession initialization warning #1518

wants to merge 2 commits into from

Conversation

AraHaan
Copy link
Contributor

@AraHaan AraHaan commented Dec 28, 2016

This fixes an issue with ClientSession throwing a warning when feeding
an event loop into it when calling it from a normal function. This
should help silence annoying warnings for libraries that "lazy"
initializes ClientSession and properly closes the session and handles
it. Now those libraries would have no need for a useless bug reports.

What do these changes do?

Fixes the warning when calling a ClientSession instance with an event loop that is passed to the class.

Are there changes in behavior for the user?

No, just adds more control over the ClientSession instance.

Related issue number

#1468 (comment)

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new entry to CHANGES.rst
    • Choose any open position to avoid merge conflicts with other PRs.
    • Add a link to the issue you are fixing (if any) using #issue_number format at the end of changelog message. Use Pull Request number if there are no issues for PR or PR covers the issue only partially.

@AraHaan
Copy link
Contributor Author

AraHaan commented Dec 28, 2016

I had to remake #1507 to fix a Merge Conflict for just 1 commit. Sorry about that.

@codecov-io
Copy link

codecov-io commented Dec 28, 2016

Current coverage is 98.92% (diff: 100%)

Merging #1518 into master will increase coverage by <.01%

@@             master      #1518   diff @@
==========================================
  Files            30         30          
  Lines          7000       7007     +7   
  Methods           0          0          
  Messages          0          0          
  Branches       1169       1170     +1   
==========================================
+ Hits           6925       6932     +7   
  Misses           37         37          
  Partials         38         38          

Powered by Codecov. Last update 49b593e...78dc630

@AraHaan AraHaan mentioned this pull request Dec 29, 2016
2 tasks
@@ -54,7 +54,7 @@ def __init__(self, *, connector=None, loop=None, cookies=None,
if loop.get_debug():
self._source_traceback = traceback.extract_stack(sys._getframe(1))

if not loop.is_running():
if not loop.is_running() and loop is not None:
Copy link
Member

Choose a reason for hiding this comment

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

Check for None shoud be first: it's fastpath.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed an issue in the None check I wrote is not none instead of is None.

Copy link
Contributor Author

@AraHaan AraHaan Jan 4, 2017

Choose a reason for hiding this comment

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

uh, so I tested it and sadly the final for loop line must be if loop is not None: because otherwise if I check if the loop is running with the current None check it would raise an AttributeError.

This fixes an issue with ClientSession throwing a warnings when feeding
an event loop into it when calling it from a normal function. This
should help silence annoying warnings for libraries that "lazy"
initializes ClientSession and properly closes the session and handles
it. Now those libraries would have no need for a useless bug reports.
@AraHaan
Copy link
Contributor Author

AraHaan commented Feb 1, 2017

Ok, So Sadly I might need to reopen this PR so this is only 1 commit. And so it would be also up to date with this repo.

@AraHaan AraHaan closed this Feb 1, 2017
@AraHaan AraHaan deleted the warnings_patch branch February 1, 2017 00:36
@AraHaan
Copy link
Contributor Author

AraHaan commented Feb 1, 2017

Remade as #1572.

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

3 participants