Marc-Andre Lemburg <m...@egenix.com> added the comment:
A view comments on the patch:
+ def __init__(self, data, encodekey, decodekey, encodevalue, decodevalue,
putenv, unsetenv):
As general guideline: When adding new parameter, please add them to the
end of the parameter list and preferably with a default argument in order
to not break the API.
Doesn't matter much in this case, since _Environ is only used internally,
but it's still good practice.
--
+data = {}
+for key, value in environ.items():
+ data[_keymap(key)] = fsencode(value)
Please put such init code into a function or make sure that the global
module space is not polluted with temporary variables such as data,
key, value.
--
+ # bytes environ
+ environb = _Environ(data, _keymap, fsencode, fsencode, fsencode, _putenv,
_unsetenv)
This looks wrong even though it will work, but that's only a
side-effect of how fsencode is coded and that's not how the
stdlib should be coded (see below).
--
+ def fsencode(value):
+ """
+ unicode to file system
+ """
+ if isinstance(value, bytes):
+ return value
+ else:
+ return value.encode(sys.getfilesystemencoding(), 'surrogateescape')
The function should not accept bytes as input or at least
document this pass-through behavior, leaving the user to decide
whether that's wanted or not.
--
The patch is also missing code which keeps the two dictionaries in
sync. If os.environ changes, os.environb would have to change as
well.
----------
_______________________________________
Python tracker <rep...@bugs.python.org>
<http://bugs.python.org/issue8603>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe:
http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
