[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

Yury Selivanov added the comment: 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. -- priority: release blocker -> normal resolution: -> fixed status: open -> closed

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

Changes by Yury Selivanov : -- nosy: +larry, ned.deily priority: normal -> release blocker ___ Python tracker ___

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

Yury Selivanov added the comment: This patch introduced multiple refleaks in test_asyncgen. -- nosy: +yselivanov resolution: fixed -> status: closed -> open ___ Python tracker

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

Roundup Robot added the comment: 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

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

Serhiy Storchaka added the comment: Thank you Martin, your comment is helpful. -- resolution: -> fixed stage: patch review -> resolved status: open -> closed ___ Python tracker

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

Roundup Robot added the comment: 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

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

STINNER Victor added the comment: Martin's comment is helpful and LGTM. -- ___ Python tracker ___ ___

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

Martin Panter added the comment: 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)

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

Serhiy Storchaka added the comment: Could you suggest concrete wording? -- ___ Python tracker ___ ___

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

Martin Panter added the comment: 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)

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

Serhiy Storchaka added the comment: Left testing only the most common cases: sys.__stdout__ is None or is non a terminal. -- Added file: http://bugs.python.org/file42529/get_terminal_size_valueerror2.patch ___ Python tracker

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

Martin Panter added the comment: 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

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

Serhiy Storchaka added the comment: Here is a patch that adds ValueError in the exceptions list and adds tests. -- resolution: fixed -> status: closed -> open Added file: http://bugs.python.org/file42525/get_terminal_size_valueerror.patch ___ Python

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

Changes by STINNER Victor : -- resolution: -> fixed status: open -> closed ___ Python tracker ___

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

STINNER Victor added the comment: 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 ;-) -- ___ Python tracker

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

Roundup Robot added the comment: 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

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

Emanuel Barry added the comment: 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

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

STINNER Victor added the comment: > 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).

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

Serhiy Storchaka added the comment: 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

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

STINNER Victor added the comment: > 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),

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

Changes by Emanuel Barry : Added file: http://bugs.python.org/file42523/get_term_size_with_test2.patch ___ Python tracker ___

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

Emanuel Barry added the comment: 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

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

Emanuel Barry added the comment: 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. -- Added file:

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

STINNER Victor added the comment: > 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,

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

STINNER Victor added the comment: get_terminal_size.diff LGTM. Would you like to try to write an unit test? Maybe using unittest.mock.patch? -- nosy: +haypo ___ Python tracker

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

Serhiy Storchaka added the comment: > 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

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

Emanuel Barry added the comment: 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

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

Martin Panter added the comment: 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

[issue26801] Fix shutil.get_terminal_size() to catch AttributeError

New submission from Emanuel Barry: `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