homepage

`shutil.get_terminal_size()` will sometimes propagate `AttributeError: module <os> 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)

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.

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.

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

get_terminal_size.diff LGTM.
Would you like to try to write an unit test? Maybe using unittest.mock.patch?

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

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.

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.

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

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.

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

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.

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 

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 ;-)

Here is a patch that adds ValueError in the exceptions list and adds tests.

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.

Left testing only the most common cases: sys.__stdout__ is None or is non a terminal.

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)

Could you suggest concrete wording?

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)

Martin's comment is helpful and LGTM.

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 

Thank you Martin, your comment is helpful.

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 

This patch introduced multiple refleaks in test_asyncgen.

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.