| msg263701 - (view) |
Author: Anilyka Barry (abarry) *  |
Date: 2016-04-18 23:43 |
| `shutil.get_terminal_size()` will sometimes propagate `AttributeError: module has not attribute 'get_terminal_size'` to the caller. The call checks for NameError, which makes no sense, so I guess it must be an oversight. Attached patch fixes it. (diff was generated with git, I don't know if it works with Mercurial, but it's a trivial change anyway) |
|
|
| msg263705 - (view) |
Author: Martin Panter (martin.panter) *  |
Date: 2016-04-19 02:53 |
| The patch looks good to me. The function was originally written to be included in the “os” module, hence the NameError. The patch should probably be fine with Mercurial, but it looks like the Reitveld review system doesn’t like it :) What platform do you get the AttributeError with? Perhaps the function is not well covered in the test suite. |
|
|
| msg263706 - (view) |
Author: Anilyka Barry (abarry) * |
Date: 2016-04-19 03:00 |
| I think Rietveld doesn't like me because I made it a .diff file, and not a .patch file, but who knows. It's a bit of a shot in the dark though, because I can't reproduce an environment where `os.get_terminal_size()` doesn't exist. I'm on Windows and sometimes compile the latest trunk to play around and find bugs, so it could be in one of those times (even though I just tried and it didn't fail). I think that if `NameError` was to be caught before, it means the function was to be "maybe there, maybe not", which could very well still be the case, so it makes sense to use the proper exception in that case. I don't see any significant drawback to catching AttributeError, anyway. |
|
|
| msg263710 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2016-04-19 05:45 |
| > What platform do you get the AttributeError with? Perhaps the function is not well covered in the test suite. I guess `os.get_terminal_size()` didn't exist on ancient OSes like DOS, OS/2, ancient UNIXes. On all supported platforms (including such exotic as old AIX, QNX or Minix) it should be defined. Tests should fail if `os.get_terminal_size()` doesn't exist. > I think Rietveld doesn't like me because I made it a .diff file, and not a .patch file, but who knows. I think this is because the patch doesn't contain a revision number. On Windows `os.get_terminal_size()` can raise ValueError if `sys.__stdout__.fileno()` is not one of 0, 1 or 2. It is worth to catch it too. An AttributeError is also raised if sys.__stdout__ has no the "fileno" attribute (StringIO or None). |
|
|
| msg263748 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2016-04-19 15:09 |
| get_terminal_size.diff LGTM. Would you like to try to write an unit test? Maybe using unittest.mock.patch? |
|
|
| msg263749 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2016-04-19 15:11 |
| > On all supported platforms (including such exotic as old AIX, QNX or Minix) it should be defined. test_shutil always call shutil.get_terminal_size() and I didn't see any failure on the unit suite on our buildbots, even less common platforms like OpenBSD, AIX, OpenIndiana, etc. So yes, os.get_terminal_size() looks to be available on all supported platforms. |
|
|
| msg263755 - (view) |
Author: Anilyka Barry (abarry) * |
Date: 2016-04-19 17:35 |
| On posix-based OSes, `os.get_terminal_size` might not exist ( https://hg.python.org/cpython/file/default/Modules/posixmodule.c#l12462 ), so this is needed. New patch file with tests included. |
|
|
| msg263756 - (view) |
Author: Anilyka Barry (abarry) * |
Date: 2016-04-19 18:07 |
| Hmm, if `sys.__stdout__` was deleted (or set to `None`), this would also raise an `AttributeError` when calling `shutil.get_terminal_size`, so I think that even if `os.get_terminal_size` is guaranteed to be always present (which it's not, IIUC), catching `AttributeError` would prevent that bug, too. Should I write a unit test for that too? Even though this one theorically covers it as well, I wouldn't want people in the future to think they can safely remove `except AttributeError` from the code. |
|
|
| msg263760 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2016-04-19 19:39 |
| > Hmm, if `sys.__stdout__` was deleted (or set to `None`), this would also raise an `AttributeError` when calling `shutil.get_terminal_size`, so I think that even if `os.get_terminal_size` is guaranteed to be always present (which it's not, IIUC), catching `AttributeError` would prevent that bug, too. It would be nice to have an unit test too for this case. You can use something like: with unittest.mock.patch('shutil.sys') as mock_sys: del mock_sys.__stdout__ print(shutil.get_terminal_size((3, 3))) (and mock also os.envion, as shown in the review.) |
|
|
| msg263762 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2016-04-19 20:12 |
| Honest, I don't think that we need such complex test for the case that isn't occurred in wild. If delete os.get_terminal_size, all TermsizeTests tests fail, so we will know if encounter a platform without os.get_terminal_size. Instead I suggest to add ValueError in exceptions list and add tests for changed sys.__stdout__: None, StringIO(), open(TESTFN, "w"). Some of these tests fail without AttributeError in the exceptions list. |
|
|
| msg263764 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2016-04-19 20:16 |
| > Honest, I don't think that we need such complex test for the case that isn't occurred in wild. Right. I'm also fine if you test manually this corner case. The Lib/shutil.py change LGTM in get_term_size_with_test2.patch (I ignored the unit test). |
|
|
| msg263766 - (view) |
Author: Anilyka Barry (abarry) * |
Date: 2016-04-19 20:20 |
| To be fair, I don't think we actually need a unit test to check if `os.get_terminal_size` exists, as we catch any `AttributeError` at all. I'd want to keep the except clause there to properly handle `sys.__stdout__` being `None` (or simply absent). I also don't consider that I'm fixing a bug here, but more like an oversight. The except clause with `NameError` is obviously an oversight from when the function was ported from `os` to `shutil`, so I'd rather fix it. |
|
|
| msg263767 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2016-04-19 20:29 |
| New changeset e3763b5964b6 by Victor Stinner in branch '3.5': Fix shutil.get_terminal_size() error handling https://hg.python.org/cpython/rev/e3763b5964b6 New changeset 75f40345d784 by Victor Stinner in branch 'default': Merge 3.5: issue #26801 https://hg.python.org/cpython/rev/75f40345d784 |
|
|
| msg263768 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2016-04-19 20:30 |
| Well, since Serhiy, Emmanuel and me agree that unit tests are overkill, I pushed the obivous and trivial fix. Thank you Emmanual for your contribution! I added your name to Misc/ACKS ;-) |
|
|
| msg263773 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2016-04-19 21:15 |
| Here is a patch that adds ValueError in the exceptions list and adds tests. |
|
|
| msg263783 - (view) |
Author: Martin Panter (martin.panter) * |
Date: 2016-04-19 22:33 |
| I doubt it is worth spending much effort supporting sys.__stdout__ being overwritten with StringIO or deleted. That seems an abuse of the “sys” module. Idle doesn’t even seem to alter this attribute. But if you call stdout.close() or detach(), I think that is a more valid way to trigger ValueError, so Serhiy’s patch is worthwhile for that case. |
|
|
| msg263804 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2016-04-20 07:17 |
| Left testing only the most common cases: sys.__stdout__ is None or is non a terminal. |
|
|
| msg264093 - (view) |
Author: Martin Panter (martin.panter) * |
Date: 2016-04-24 04:40 |
| Serhiy’s patch looks worthwhile to me, though I still think a comment would help. There are lots of different cases being handled by those few lines: try: size = os.get_terminal_size(sys.__stdout__.fileno()) except (AttributeError, ValueError, OSError) |
|
|
| msg264095 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2016-04-24 05:09 |
| Could you suggest concrete wording? |
|
|
| msg264096 - (view) |
Author: Martin Panter (martin.panter) * |
Date: 2016-04-24 06:22 |
| How about: try: size = os.get_terminal_size(sys.__stdout__.fileno()) except (AttributeError, ValueError, OSError): # stdout is None, closed, detached, or not a terminal, or # os.get_terminal_size() is unsupported size = os.terminal_size(fallback) |
|
|
| msg264097 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2016-04-24 06:34 |
| Martin's comment is helpful and LGTM. |
|
|
| msg264099 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2016-04-24 06:59 |
| New changeset df8652452d25 by Serhiy Storchaka in branch '3.5': Issue #26801: shutil.get_terminal_size() now handles the case of stdout is https://hg.python.org/cpython/rev/df8652452d25 New changeset d6e6dcef674f by Serhiy Storchaka in branch 'default': Issue #26801: shutil.get_terminal_size() now handles the case of stdout is https://hg.python.org/cpython/rev/d6e6dcef674f |
|
|
| msg264100 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2016-04-24 07:00 |
| Thank you Martin, your comment is helpful. |
|
|
| msg278347 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2016-10-09 05:51 |
| New changeset 678424183b38 by INADA Naoki in branch '3.6': Issue #26801: Added C implementation of asyncio.Future. https://hg.python.org/cpython/rev/678424183b38 New changeset f8815001a390 by INADA Naoki in branch 'default': Issue #26801: Added C implementation of asyncio.Future. https://hg.python.org/cpython/rev/f8815001a390 |
|
|
| msg280357 - (view) |
Author: Yury Selivanov (yselivanov) * |
Date: 2016-11-08 22:20 |
| This patch introduced multiple refleaks in test_asyncgen. |
|
|
| msg280358 - (view) |
Author: Yury Selivanov (yselivanov) * |
Date: 2016-11-08 22:22 |
| Ah, never mind, the commit message has a wrong issue number: Issue #26801: Added C implementation of asyncio.Future. Closing this one, will re-open #26081. |
|
|