|
|
|
Add BaseEventLoop._closed attribute
Patch Set 1 #
Total comments: 9
Patch Set 2 : new BaseEventLoop.is_closed() method #
Total messages: 8
|
GvR
https://codereview.appspot.com/98680044/diff/1/asyncio/base_events.py File asyncio/base_events.py (right): https://codereview.appspot.com/98680044/diff/1/asyncio/base_events.py#newcode135 asyncio/base_events.py:135: self._closed, self.get_debug())) I wonder if we should also add ...
|
11 years, 7 months ago (2014年05月28日 23:05:27 UTC) #1 | ||||||||||||||||||||||||||||||||||||||||||||||||
https://codereview.appspot.com/98680044/diff/1/asyncio/base_events.py File asyncio/base_events.py (right): https://codereview.appspot.com/98680044/diff/1/asyncio/base_events.py#newcode135 asyncio/base_events.py:135: self._closed, self.get_debug())) I wonder if we should also add a property or accessor method so a user can test the closed flag. There are a lot of places in the patch now that use this private variable. https://codereview.appspot.com/98680044/diff/1/asyncio/proactor_events.py File asyncio/proactor_events.py (right): https://codereview.appspot.com/98680044/diff/1/asyncio/proactor_events.py#new... asyncio/proactor_events.py:356: if self._closed: I'm not sure if this is an improvement. Now you are depending on an implementation detail of the superclass. https://codereview.appspot.com/98680044/diff/1/asyncio/selector_events.py File asyncio/selector_events.py (right): https://codereview.appspot.com/98680044/diff/1/asyncio/selector_events.py#new... asyncio/selector_events.py:58: if self._closed: Ditto. https://codereview.appspot.com/98680044/diff/1/tests/test_selector_events.py File tests/test_selector_events.py (left): https://codereview.appspot.com/98680044/diff/1/tests/test_selector_events.py#... tests/test_selector_events.py:95: def test_close_no_selector(self): Did you mean to delete this test? https://codereview.appspot.com/98680044/diff/1/tests/test_selector_events.py File tests/test_selector_events.py (right): https://codereview.appspot.com/98680044/diff/1/tests/test_selector_events.py#... tests/test_selector_events.py:81: You don't need this blank line.
https://codereview.appspot.com/98680044/diff/1/asyncio/base_events.py File asyncio/base_events.py (right): https://codereview.appspot.com/98680044/diff/1/asyncio/base_events.py#newcode133 asyncio/base_events.py:133: return ('<%s running=%s closed=%s debug=%s>' Hm, maybe if event loop is closed, then we only need <EventLoop closed> form, and when it's not closed, we can shod <Event loop running=True debug=False> form? https://codereview.appspot.com/98680044/diff/1/tests/test_selector_events.py File tests/test_selector_events.py (left): https://codereview.appspot.com/98680044/diff/1/tests/test_selector_events.py#... tests/test_selector_events.py:103: self.assertIsNone(self.loop._selector) Are you sure it's a good idea to remove these tests?
https://codereview.appspot.com/98680044/diff/1/asyncio/base_events.py File asyncio/base_events.py (right): https://codereview.appspot.com/98680044/diff/1/asyncio/base_events.py#newcode135 asyncio/base_events.py:135: self._closed, self.get_debug())) On 2014年05月28日 23:05:26, GvR wrote: > I wonder if we should also add a property or accessor method so a user can test > the closed flag. There are a lot of places in the patch now that use this > private variable. File objects of the io module have a read-only closed property. We can add a get_closed() method to be consistent with other flags: get_debug() for _debug. It is useful to know if the event loop is closed or not. https://codereview.appspot.com/98680044/diff/1/tests/test_selector_events.py File tests/test_selector_events.py (left): https://codereview.appspot.com/98680044/diff/1/tests/test_selector_events.py#... tests/test_selector_events.py:103: self.assertIsNone(self.loop._selector) On 2014年05月28日 23:07:09, yselivanov wrote: > Are you sure it's a good idea to remove these tests? These checks should be moved to test_close().
Since event loops don't have any other properties I think get_closed() is the thing to do.
On 2014年05月28日 23:45:03, GvR wrote: > Since event loops don't have any other properties I think get_closed() is the > thing to do. Maybe is_closed(), similar to is_running()?
Oh, fine. On Wed, May 28, 2014 at 4:50 PM, <yselivanov@gmail.com> wrote: > On 2014年05月28日 23:45:03, GvR wrote: > >> Since event loops don't have any other properties I think get_closed() >> > is the > >> thing to do. >> > > Maybe is_closed(), similar to is_running()? > > https://codereview.appspot.com/98680044/ > -- --Guido van Rossum (python.org/~guido)
LGTM. You may shorten the __repr__ if not closed (as Yuri suggested), or you may leave it as is.
On 2014年06月03日 03:11:34, GvR wrote: > LGTM. You may shorten the __repr__ if not closed (as Yuri suggested), or you may > leave it as is. I disagree with Yury's suggestion for __repr__ because it is possible to close an event loop while it is running (running=True). But it looks like a bug in asyncio: https://code.google.com/p/tulip/issues/detail?id=171 I will reconsider Yury's suggestion when this issue will be solved.