Author zbysz
Recipients Arfrever, amaury.forgeotdarc, denilsonsa, eric.araujo, giampaolo.rodola, loewis, neologix, pitrou, rosslagerwall, techtonik, vstinner, zbysz
Date 2012-01-31.16:36:32
SpamBayes Score 1.66533e-16
Marked as misclassified No
Message-id <4F281887.2010906@in.waw.pl>
In-reply-to <1328019747.05.0.0543783154098.issue13609@psf.upfronthosting.co.za>
Content
Well, right now it's just one function. Functionality which you propose
could of course be useful, but let's leave if for another day, another 
issue.

See also http://bugs.python.org/issue13609#msg149627 and #444582 and 
#12442 -- shutil is growing in different directions, and terminal size 
can be one of them.

 > Antoine Pitrou added the comment:
 > "except Exception" clauses in the tests are too broad.
Fixed.
 > Otherwise, looks fine.

 > STINNER Victor added the comment:
 > - shutil.get_terminal_size(): I would prefer columns and lines
 > variable names instead of "ccc" and "rrr"
Changed (this was a leftover from the version when there was no named 
tuple).

 > - I would prefer os.get_terminal_size() instead of
 > os.query_terminal_size(), I didn't know other function using the verb
 > "query" in Python
Changed. I used os.g_t_s and shutil.g_t_s in docs so it should be clear 
which is which.

 > - To keep os simple, os.query_terminal_size() can return a simple,
 > whereas shutil.get_terminal_size() returns a namedtuple
We would have two very similar functions returning something different. 
Also, the amount of code saved will be negligible, because the named 
tuple is mostly docs. I prefer to keep it in os to keep both functions 
similar.

 > - test_os.py: use @unittest.skipUnless() on TermsizeTests to check if
 > os.query_terminal_size() is available
> - To keep os simple, os.query_terminal_size() can return a simple,
> whereas shutil.get_terminal_size() returns a namedtuple
> AttributeError should be catched here, not NameError. Or better, check
> if os has a query_terminal_size() function.
Fixed. I changed the tests a bit, e.g. to take into account that stty 
queries stdin, not stdout.

 > Hum, you may document the default value: (80, 24).
Done.

shutil.get_terminal_size() is tied to COLUMNS and ROWS and thus makes 
most sense for stdout. But I guess that an optional parameter could be 
added:
   shutil.get_terminal_size(fd=None, fallback=(80, 24))
where fd could be either an integer, or an object with .fileno().
I don't know.

 > - "Right now the values are not cached, but this might change." why
 > would it be cached? this sentence can just be removed
Done. In theory it could be cached with watching for SIGWINCH, but I'll 
just remove the comment for now.

Thanks for the review and comments!

Patch version nine attached: termsize.diff.8
Files
File name Uploaded
termsize.diff.8 zbysz, 2012-01-31.16:36:30
History
Date User Action Args
2012-01-31 16:36:34zbyszsetrecipients: + zbysz, loewis, amaury.forgeotdarc, pitrou, vstinner, techtonik, giampaolo.rodola, eric.araujo, Arfrever, denilsonsa, neologix, rosslagerwall
2012-01-31 16:36:33zbyszlinkissue13609 messages
2012-01-31 16:36:32zbyszcreate