STINNER Victor added the comment:
Comments on add_os_release_support_2.patch:
- You should not write a huge try/except OSError block. I would prefer
something like:
try:
f = open(...)
except OSError:
return None
with f:
...
- I'm not sure about that, but you might use errors='surrogateescape', just in
case if the file is not correctly encoded. Surrogate characters are maybe less
annoying than a huge UnicodeDecodeError
- You use /etc/os-release even if the file is empty. Do you know if there is a
standard, or something like that explaining if some keys are mandatory? For
example, we can ignore os-release if 'ID' or 'VERSION_ID' key is missing
- For the unit test, you should write at least a test on linux_distribution()
to check that your private function is used correctly
- You add unit tests for all escaped characters (', ", \), for comments, and
maybe also for maformated lines (to have a well defined behaviour, ignore them
or raise an error)
- _UNIXCONFDIR looks to be dedicated to unit tests, can't you patch builtin
open() instead? It would avoid the need of a temporary directory
----------
_______________________________________
Python tracker <[email protected]>
<http://bugs.python.org/issue17762>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe:
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
