Zbyszek Szmek <zbys...@in.waw.pl> added the comment:
Thanks for the review!
New version is attached. The code is actually slightly shorter, but
there are more docs.
Doc/library/os.rst | 52 +++++++++++++++++++
Doc/whatsnew/3.3.rst | 5 +
Lib/os.py | 43 +++++++++++++++
Lib/test/test_termsize.py | 31 +++++++++++
Misc/NEWS | 3 +
Modules/posixmodule.c | 125 ++++++++++++++++++++++++++++++++++++++++++++++
configure | 2
configure.in | 2
pyconfig.h.in | 3 +
9 files changed, 264 insertions(+), 2 deletions(-)
>The patch also lacks some documentation update in Doc/library/os.rst.
Added as a subsection under "File Descriptor Operations" section.
I also added an entry to Misc/NEWS and Doc/whatsnew/3.3.rst.
> 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.
The implementation was borked and didn't actually do what the
docstring said. It should be fixed now.
I want to retain the default values, because get_terminal_size()
should "do the right thing" in case stdout is not a terminal,
without the user needing to wrap it in a try..except block
or some other test. Putting the default values as parameters
makes it clear what will be returned in case query fails, and
makes it easy to override those defaults.
To make it clearer that those are the fallback values, I renamed the
parameter to 'fallback=(c, r)', which should be understandable even
without reading the docstring. Having it as a single parameter might
also make it easier to add something like 'minimum=(c, r)' in the
future.
> 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.
OK, changed to sys.__stdout__.fileno().
I think that sys.__stdout__ is better, because when stdout is overridden,
it is usually with something that is not a terminal. Changing the output
terminal is probably pretty rare.
> Actually, perhaps get_terminal_size() should have an optional file descriptor
> argument.
Querying anything other than stdout should be pretty rare. If
necessary, query_terminal_size() can be called explicitly. Also,
the variables $COLUMNS and $ROWS are usually meant to refer to stdout,
so get_terminal_size() is about stdout and ROWS and COLUMNS, and the
other function allows full control.
> Lib/test/test_termsize.py:1: import unittest
> This file seems to lack an ``if __name__ == '__main__'`` like other test files
OK.
> Besides, why not simply put the tests in test_os.py?
The tests were nicely self-contained. I wanted to be able to do:
./python Lib/test/test_termsize.py
and
./python Lib/test/test_termsize.py | cat
but I guess it can be easily moved to test_os.py if necessary.
> Lib/test/test_termsize.py:7: def set_environ(**variables):
> There's already EnvironmentVarGuard in test.support.
OK.
> Lib/test/test_termsize.py:7: def set_environ(**variables):
> There's already EnvironmentVarGuard in test.support.
> Lib/test/test_termsize.py:25: self.assertTrue(0 < size.columns)
> Better to use assertGreater here, failure messages will be more informative.
> Lib/test/test_termsize.py:33: self.assertTrue(size.columns == 777)
> And better to use assertEqual here.
OK.
> Typo here ("imlmented").
OK.
> Modules/posixmodule.c:10571: return PyErr_Format(PyExc_ValueError)
> I don't think there's any point in changing the error here. Just let
> a normal OSError be raised.
OK, s/ValueError/OSError/ here and the the windows case below too.
> Modules/posixmodule.c:10573: return PyErr_SetFromErrno(PyExc_IOError);
> For the record, in 3.3, PyExc_IOError is an alias of PyExc_OSError.
OK, s/IOError/OSError/ for clarity.
> Modules/posixmodule.c:10599: return PyErr_Format(PyExc_IOError, "error %i",
> (int) GetLastError());
> Just use PyErr_SetFromWindowsErr(GetLastError()).
OK.
> 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.
The idea is that query_terminal_size() can be used to do the
low-level query ignoring $COLUMNS and $ROWS and returing the real
error if something goes wrong. If is documented, so I think that
it can be "public".
I've also improved the docstrings slightly.
----------
Added file: http://bugs.python.org/file24117/termsize.diff.2
_______________________________________
Python tracker <rep...@bugs.python.org>
<http://bugs.python.org/issue13609>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe:
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
