Skip to content

bpo-32418: Add get_loop() method on Server, AbstractServer classes#4997

Merged
asvetlov merged 4 commits intopython:masterfrom
srinivasreddy:32418
Dec 30, 2017
Merged

bpo-32418: Add get_loop() method on Server, AbstractServer classes#4997
asvetlov merged 4 commits intopython:masterfrom
srinivasreddy:32418

Conversation

@srinivasreddy
Copy link
Copy Markdown
Contributor

@srinivasreddy srinivasreddy commented Dec 23, 2017 •
edited by bedevere-bot
Loading

@1st1
Copy link
Copy Markdown
Member

1st1 commented Dec 23, 2017

Please add tests/NEWS entry/docs.

1st1
1st1 reviewed Dec 23, 2017
View reviewed changes
Comment thread Lib/asyncio/events.py Outdated
Copy link
Copy Markdown
Member

@1st1 1st1 Dec 23, 2017

Choose a reason for hiding this comment

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

""" Get the event loop""" -> """Get the event loop the Server object is attached to."""

@srinivasreddy srinivasreddy changed the title bpo-32418: Add abstract get_loop() method bpo-32418: Add get_loop() method on Server, AbstractServer classes Dec 23, 2017
asvetlov
asvetlov requested changes Dec 23, 2017
View reviewed changes
Comment thread Misc/NEWS.d/next/Documentation/2017-12-24-17-29-37.bpo-32418.eZe-ID.rst Outdated
Copy link
Copy Markdown
Contributor

@asvetlov asvetlov Dec 23, 2017

Choose a reason for hiding this comment

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

Drop bpo number: it will be added automatically by blurb.

Comment thread Lib/asyncio/base_events.py Outdated
Copy link
Copy Markdown
Contributor

@asvetlov asvetlov Dec 23, 2017

Choose a reason for hiding this comment

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

Please add a test for the method.

Copy link
Copy Markdown
Contributor Author

@srinivasreddy srinivasreddy Dec 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I cannot find tests for Server class. Could you help me please? Or it looks like i need to write test cases for Server class.

Copy link
Copy Markdown
Member

@1st1 1st1 Dec 23, 2017

Choose a reason for hiding this comment

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

loop.create_server returns Server objects.

Comment thread Lib/asyncio/events.py Outdated
Copy link
Copy Markdown
Contributor

@asvetlov asvetlov Dec 23, 2017

Choose a reason for hiding this comment

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

Please add a test for make sure that AbstractServer().get_loop() returns NotImplemented.

Copy link
Copy Markdown
Member

@1st1 1st1 Dec 30, 2017

Choose a reason for hiding this comment

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

@asvetlov It should raise NotImplementedError, please fix. NotImplemened singleton is for comparisons in Python.

Copy link
Copy Markdown
Contributor

@asvetlov asvetlov Dec 30, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Oooh, my bad.

@bedevere-bot
Copy link
Copy Markdown

bedevere-bot commented Dec 23, 2017

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

asvetlov
asvetlov reviewed Dec 25, 2017
View reviewed changes
Comment thread Lib/test/test_asyncio/test_events.py Outdated
Copy link
Copy Markdown
Contributor

@asvetlov asvetlov Dec 25, 2017

Choose a reason for hiding this comment

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

The method is a coroutine, use server = loop.run_until_complete(loop.create_server(lambda: proto, '0.0.0.0', 0))

asvetlov
asvetlov reviewed Dec 25, 2017
View reviewed changes
Comment thread Lib/test/test_asyncio/test_events.py Outdated
Copy link
Copy Markdown
Contributor

@asvetlov asvetlov Dec 25, 2017

Choose a reason for hiding this comment

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

The call is not needed but loop should be closed after the test.

@srinivasreddy
Copy link
Copy Markdown
Contributor Author

srinivasreddy commented Dec 26, 2017 •
edited
Loading

@asvetlov Done. Thanks!! Do i need to update the documentation? Looks like it.

@srinivasreddy
Copy link
Copy Markdown
Contributor Author

srinivasreddy commented Dec 26, 2017

@asvetlov I have updated the docs, pls let me know what do you think?

@asvetlov
Copy link
Copy Markdown
Contributor

asvetlov commented Dec 27, 2017

Should be documented not AbstractEventLoop.create_server but Server here: https://docs.python.org/3.7/library/asyncio-eventloop.html#server

@srinivasreddy
Copy link
Copy Markdown
Contributor Author

srinivasreddy commented Dec 29, 2017

@asvetlov Done.

@asvetlov asvetlov merged commit 1634fc2 into python:master Dec 30, 2017
@asvetlov
Copy link
Copy Markdown
Contributor

asvetlov commented Dec 30, 2017

Thanks!

1st1
1st1 reviewed Dec 30, 2017
View reviewed changes
Comment thread Lib/test/test_asyncio/test_events.py
proto = MyProto(loop)
server = loop.run_until_complete(loop.create_server(lambda: proto, '0.0.0.0', 0))
self.assertEqual(server.get_loop(), loop)
loop.close()
Copy link
Copy Markdown
Member

@1st1 1st1 Dec 30, 2017

Choose a reason for hiding this comment

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

This test will leak an open server object. Add server.close(); loop.run_until_complete(server.wait_closed())

@1st1
Copy link
Copy Markdown
Member

1st1 commented Dec 30, 2017

@asvetlov There are a couple of issues with this PR, please fix if possible.

@asvetlov asvetlov mentioned this pull request Dec 30, 2017
Merged
@srinivasreddy srinivasreddy deleted the 32418 branch December 30, 2017 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@1st1 1st1 1st1 left review comments

@asvetlov asvetlov asvetlov approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@srinivasreddy @1st1 @bedevere-bot @asvetlov @the-knights-who-say-ni