Author zbysz
Recipients Arfrever, amaury.forgeotdarc, denilsonsa, giampaolo.rodola, loewis, neologix, pitrou, rosslagerwall, zbysz
Date 2012-01-02.18:57:24
SpamBayes Score 0.0
Marked as misclassified No
Message-id <1325530645.89.0.518264793167.issue13609@psf.upfronthosting.co.za>
In-reply-to
Content
Thanks for the reviews!

> - build the namedtuple in Python rather than in C
I started this way, but if two functions are used, it is nicer to have them both
return the same type. If it was defined in the Python part, there would be
loop in the sense that os would import the function from _posixmodule, and
_posixmodule would import the namedtuple from os. The code for the tuple is fairly simple,
mostly docs, and saving a few lines just doesn't seem to be worth the complication of doing
it other way around.

> - I don't particularly like CamelCased names for nametuples (I would use "size" instead of "TerminalSize")
I was following the style in _posixmodule: there is WaitidResultType and SchedParamType.
"size" seems to generic, maybe something like "terminal_size" would be better (SchedParamType
is exported as sched_param).

Will change to "terminal_size".

- on UNIX, the ioctl() stuff can be done in python rather than in C
It can, but it requires extracting the bytes by hand, doing it in C is cleaner.
The C implementation is only 5 lines (in addition to the code necessary for windows)
and again it seems simpler this way. Also the configuration is kept in one
place in the #ifdefs at the top, not in two different files.

> - use C only on Windows to deal with GetStdHandle/GetConsoleScreenBufferInfo; function name should be
> accessed as nt._get_terminal_size similarly to what we did for shutil.disk_usage in issue12442.
> - do not raise NotImplementedError; if the required underlying functions are not available 
> os.get_terminal_size should not exists in the first place (same as we did for shutil.disk_usage / issue12442)

I think that there is a difference in "importance" -- shutil.disk_usage cannot be
faked, or ignored, and if it is unavailable, then some work-around must be implemented.
OTOH, the terminal size is an embellishment, and if a wrong terminal size is used, the output
might wrap around or be a bit narrow (the common case), but like in argparse.FancyFormatter,
just continuing is probably reasonable. So to save the users of the function the trouble
to wrap it in some try..except, let's cater for the common case.

This same argument goes for defining the function only if an implementation is available:
argparse would need something like:
try:
  os.get_terminal_size
except NameError:
  def get_terminal_size():
    return (80, 25)
which just doesn't seem to _gain_ anything.

> Also, I'm not sure I like fallback=(80, 25) as default, followed by:
> [...]
> That means we're never going to get an exception.
> Instead I would:
> - provide fallback as None
> - let OSError propate unless fallback is not None
Again, if the user forgets to add the fallback, and only tests in a terminal, she 
would get an unnecessary surprise when running the thing in a pipe. So the fallback
would have to be almost always provided... and it will almost always be (80, 25).
So let's just provide it upfront, and let the user call the low-level function if
they want full control.

-----------------------------------------------------------------------------------

> I think the two function approach works well.
:)

> Since you have already checked that termsize is not NULL, Py_DECREF can be used instead of Py_CLEAR.
OK.

> Would it not be better to use sys.__stdout__ instead of 1 in the documentation and to use STDOUT_FILENO 
> instead of 1 in the code?
OK.

> A brief google suggested that Solaris requires sys/termios.h for TIOCGWINSZ. Perhaps also only define
> TERMSIZE_USE_IOCTL if TIOCGWINSZ is defined.
Hm, I don't have solaris available, so I won't be able to check easily, but
maybe sys/termios.h should be imported if TIOCGWINSZ is not available. Would 
be nice to provide the implementation if possible.

> Like so:
Something like:

> #if defined(HAVE_SYS_IOCTL_H)
> #include <sys/ioctl.h>
> #if defined(TIOCGWINSZ)
> #define TERMSIZE_USE_IOCTL
> #else

#if defined(HAVE_SYS_TERMIOS_H)
#include <sys/termios.h>
#if defined(TIOCGWINSZ)
#define TERMSIZE_USE_IOCTL
#else
#define TERMSIZE_USE_NOTIMPLEMENTED

> #endif
> #elif defined(HAVE_CONIO_H)
> #include <windows.h>
> #include <conio.h>
> #define TERMSIZE_USE_CONIO
> #else
> #define TERMSIZE_USE_NOTIMPLEMENTED
> #endif
History
Date User Action Args
2012-01-02 18:57:25zbyszsetrecipients: + zbysz, loewis, amaury.forgeotdarc, pitrou, giampaolo.rodola, Arfrever, denilsonsa, neologix, rosslagerwall
2012-01-02 18:57:25zbyszsetmessageid: <1325530645.89.0.518264793167.issue13609@psf.upfronthosting.co.za>
2012-01-02 18:57:25zbyszlinkissue13609 messages
2012-01-02 18:57:24zbyszcreate