Repository navigation
Add and use new EnvironDict class - #744
Conversation
ec349de to
9359153
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #744 +/- ##
==========================================
- Coverage 87.66% 87.23% -0.43%
==========================================
Files 74 75 +1
Lines 4442 4686 +244
Branches 771 816 +45
==========================================
+ Hits 3894 4088 +194
- Misses 433 468 +35
- Partials 115 130 +15 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
d38796c to
6935571
Compare
|
Tapping Pepper for review here (via @claraberendsen) |
knmcguire
left a comment
There was a problem hiding this comment.
lgtm! and your explanation makes sense
The one thing perhaps to mention is that 'move_to_end()' from OrderedDict is not changed and can't handle the case-sensitive cases. It's not used anywhere though... so... I don't think it has consequences now?
fyi, did you use any agents here? It's not mentioned in the commits
On Win32, the `os.environ` mapping behaves in many case-insensitive ways. When we copy `os.environ` to a typical `dict` instance, we lose those behaviors. This new class preserves the case insensitivity on Windows and behaves like a normal OrderedDict on non-Windows platforms. Assisted-by: Gemini 3.5 Flash <gemini@google.com>
6935571 to
c589bee
Compare
An oversight, addressed in the latest change.
I did and I definitely forgot to mention it. Good call out. |
On Win32, the
os.environmapping behaves in many case-insensitive ways. When we copyos.environto a typicaldictinstance, we lose those behaviors.This new class preserves the case insensitivity on Windows and behaves like a normal OrderedDict on non-Windows platforms.
In draft for now, because I'm still trying to understand the exact behaviors that Windows exhibits. In particular, it might be closer in behavior to just convert all keys to UPPERCASE before accessing the underlying dict.Okay, from what I can tell, Python converts all keys in
os.environto UPPERCASE on Windows, but since colcon is capturing the environment variables usingset, we don't see the same treatment. If the goal was to alignEnvironDictwithos.environon Windows precisely, we'd want to convert everything to UPPERCASE as it seems to implicitly do. However, given that we want this class to be used on the variables we capture withsetas well, I think we should just preserve the original case but make access case-insensitive.Assisted-by: Gemini 3.5 Flash
Note
The Windows implementation of
EnvironDictis a lot bigger than it should be. ImplementingMappingwas a lot simpler, but the public colcon API specifically returns an instance ofdict, so we need the new class to also inherit fromdictto maintain compatibility.