Zbyszek Szmek <zbys...@in.waw.pl> added the comment: 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 ---------- _______________________________________ 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