bpo-36144: Update os.environ and os.environb for PEP 584 #18911
bpo-36144: Update os.environ and os.environb for PEP 584 #18911gvanrossum merged 10 commits intopython:masterfrom
Conversation
brandtbucher
left a comment
brandtbucher
left a comment
There was a problem hiding this comment.
Thanks for your time @chaburkland, and welcome to CPython! 😉
Just a couple of minor minor comments, but otherwise this looks good:
|
@chaburkland It looks like the environment isn't updating properly on the Windows builds. I think this is because Windows environment variables must be uppercased! Updating all of your test data with UPPERCASED keys should fix the failing tests. |
Co-Authored-By: Brandt Bucher <brandtbucher@gmail.com>
Co-Authored-By: Brandt Bucher <brandtbucher@gmail.com>
Co-Authored-By: Brandt Bucher <brandtbucher@gmail.com>
Lib/test/test_os.py
Outdated
| os.environ[overridden_key] = 'original_value' | ||
|
|
||
| new_vars_dict = {'_A_': '1', '_B_': '2', overridden_key: '3'} | ||
| expected = os.environ.copy() |
There was a problem hiding this comment.
Maybe dict(os.environ) to emphasize that we expect the actual value to be a plain dict?
|
|
||
| actual = os.environ | new_vars_dict | ||
| self.assertDictEqual(expected, actual) | ||
| self.assertEqual('3', actual[overridden_key]) |
There was a problem hiding this comment.
Can you find a way to test whether this operation has a side effect on the process environment? (It shouldn't have. But it should for __ior__.)
There was a problem hiding this comment.
Good point. I extended the tests to check this is behaving as expected.
gvanrossum
left a comment
gvanrossum
left a comment
There was a problem hiding this comment.
LGTM. I just wonder if we can save a few subprocesses by not testing for _B_.
Lib/test/test_os.py
Outdated
| self.assertIs(NotImplemented, os.environ.__or__(new_vars_items)) | ||
|
|
||
| self._test_underlying_process_env('_A_', '') | ||
| self._test_underlying_process_env('_B_', '') |
There was a problem hiding this comment.
Do we gain anything by testing for _B_? It seems it's handled exactly the same as _A_. (I'm asking because this test runs a subprocess, which is pretty expensive compared to almost everything else these tests do.)
There was a problem hiding this comment.
Another option could be checking all three with one subprocess.
There was a problem hiding this comment.
But that would uglify the code. What's gained by checking three instead of two?
There was a problem hiding this comment.
I don't think anything is gained other than the code "feeling" complete in what it's testing. Admittedly, it's not actually testing different behavior.
The additional subprocess calls aren't very expensive though - it's only adding between 10 & 20 milliseconds. That being said, I don't have a strong feeling about it, so I'd be willing to remove the _B_ tests.
There was a problem hiding this comment.
It's still something, and not everybody's machine is as fast. Let's cut the _B_ tests -- it's too high a price for a sense of completeness or symmetry.
|
@gvanrossum: Please replace |
|
Congrats on your first CPython contribution @chaburkland! 🍾 Looking forward to seeing more from you in the future. |
|
@chaburkland Congrats indeed! Great job. |
5 participants
@brandtbucher
When implementing
__or__and__ror__, I found there to be two potential candidates for a return value, either adict, or anos._Environclass.I decided on returning a
dictfor two reaons.copy()methodos._Environobjects that are not in sync with each other.https://bugs.python.org/issue36144