Message 152394 - Python tracker (original) (raw)

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