Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(128938)

#13609: Add "os.get_terminal_size()" function

Can't Edit
Can't Publish+Mail
Start Review
Created:
7 years, 11 months ago by denilsonsa
Modified:
7 years, 9 months ago
Reviewers:
pitrou
CC:
loewis, amaury.forgeotdarc, AntoinePitrou, haypo, techtonik, giampaolo.rodola, eric.araujo, Arfrever, Z. Jędrzejewski-Szmek, denilsonsa_gmail.com, Charles-François Natali, rosslagerwall, devnull_psf.upfronthosting.co.za
Visibility:
Public.

Patch Set 1 #

Patch Set 2 #

Total comments: 11

Patch Set 3 #

Patch Set 4 #

Patch Set 5 #

Unified diffs Side-by-side diffs Delta from patch set Stats Patch
Lib/shutil.py View 1 chunk +1 line, -1 line 0 comments Download
Modules/posixmodule.c View 2 3 4 3 chunks +22 lines, -3 lines 0 comments Download

Messages

Total messages: 1
AntoinePitrou
7 years, 10 months ago #1
Hello,

Thanks for the patch. Here is a more detailed review.
The patch also lacks some documentation update in Doc/library/os.rst.

http://bugs.python.org/review/13609/diff/3838/12708
File Lib/os.py (right):

http://bugs.python.org/review/13609/diff/3838/12708#newcode833
Lib/os.py:833: def get_terminal_size(columns=80, rows=25):
The arguments seem ignored in the function body, so I think they should simply
be removed.

http://bugs.python.org/review/13609/diff/3838/12708#newcode862
Lib/os.py:862: size = query_terminal_size()
I wonder if you shouldn't try to explicitly pass sys.stdout.fileno() here, or
perhaps sys.__stdout__.fileno() since the former could be overriden.

Actually, perhaps get_terminal_size() should have an optional file descriptor
argument.

http://bugs.python.org/review/13609/diff/3838/12709
File Lib/test/test_termsize.py (right):

http://bugs.python.org/review/13609/diff/3838/12709#newcode1
Lib/test/test_termsize.py:1: import unittest
This file seems to lack an ``if __name__ == '__main__'`` like other test files
have. This means nothing happens when you run e.g. ``./python -m test -v
test_termsize``.

Besides, why not simply put the tests in test_os.py?

http://bugs.python.org/review/13609/diff/3838/12709#newcode7
Lib/test/test_termsize.py:7: def set_environ(**variables):
There's already EnvironmentVarGuard in test.support.

http://bugs.python.org/review/13609/diff/3838/12709#newcode25
Lib/test/test_termsize.py:25: self.assertTrue(0 < size.columns)
Better to use assertGreater here, failure messages will be more informative.

http://bugs.python.org/review/13609/diff/3838/12709#newcode33
Lib/test/test_termsize.py:33: self.assertTrue(size.columns == 777)
And better to use assertEqual here.

http://bugs.python.org/review/13609/diff/3838/12710
File Modules/posixmodule.c (right):

http://bugs.python.org/review/13609/diff/3838/12710#newcode10562
Modules/posixmodule.c:10562: "not imlemented for this system, sorry");
Typo here ("imlmented").

http://bugs.python.org/review/13609/diff/3838/12710#newcode10571
Modules/posixmodule.c:10571: return PyErr_Format(PyExc_ValueError, "bad file
descriptor");
I don't think there's any point in changing the error here. Just let a normal
OSError be raised.

http://bugs.python.org/review/13609/diff/3838/12710#newcode10573
Modules/posixmodule.c:10573: return PyErr_SetFromErrno(PyExc_IOError);
For the record, in 3.3, PyExc_IOError is an alias of PyExc_OSError.

http://bugs.python.org/review/13609/diff/3838/12710#newcode10599
Modules/posixmodule.c:10599: return PyErr_Format(PyExc_IOError, "error %i",
(int) GetLastError());
Just use PyErr_SetFromWindowsErr(GetLastError()).

http://bugs.python.org/review/13609/diff/3838/12710#newcode11086
Modules/posixmodule.c:11086: {"query_terminal_size", query_terminal_size,
METH_VARARGS, termsize__doc__},
I don't think there's any point in making this helper function public, so I'd
rename it _query_terminal_size or something.
Sign in to reply to this message.

RSS Feeds Recent Issues | This issue
This is Rietveld 894c83f36cb7+