Fix crash since os.environ can't be pickled#23
Conversation
|
Can you please add description why |
rotu
force-pushed
the
dirk-thomas/improve-python-package-information
branch
from
1170c23 to
1c343a7
Compare
|
good call - I wasn't clear. I moved it, switched to the dict() constructor, and spelled it out a bit better. |
|
CI error is because line length. I'd add a type annotation instead of a comment, but maybe you don't like them. |
|
|
||
| :param Path setup_py: path to a setup.py script | ||
| :param dict env: environment variables to set before running setup.py | ||
| :param Mapping[str, str] env: environment variables to set before running setup.py |
There was a problem hiding this comment.
I don't think Mapping is a defined type in this context at the moment. Should it be collections.abc.Mapping?
Are there any cases where this is called with a dict?
There was a problem hiding this comment.
typing.Mapping[str, str]. If you try to use collections.abc, you'll get an error.
I don't know if there are cases now, but you want to allow a dict, so that it can be modified before passing. Changing os.environ is probably not a good idea.
That should be straight forward by wrapping some part of the line to the next line.
Currently most of the API doesn't use it (since only newer Python version support more complete type annotations). As long as it is compatible with Python 3.5 I am fine with adding it. |
rotu
force-pushed
the
dirk-thomas/improve-python-package-information
branch
from
1c343a7 to
40fd670
Compare
|
Thanks for the patch. |
dirk-thomas
merged commit 7c8df83
into
colcon:dirk-thomas/improve-python-package-information
2 participants
No description provided.