matklad / xshell

Apache License 2.0
696 stars 30 forks source link

Return the environment variables currently known to shell #78

Open bzm3r opened 10 months ago

bzm3r commented 10 months ago

Addresses #77

TO DETERMINE:

matklad commented 10 months ago

Yes, this makes sense to me! I paused a bit on the allocating API, but I think that's fine --- the std forces an allocation as well, and, while it has a separate itearator, I don't think that's going to be much of a win over returning a vector.

However, let's sort the vector before returning, to increase determinism, like we do in read_dir. We don't aim to be zero-overhead, but we do want to be robust, and being deterministic is pretty important for robustness.

bzm3r commented 10 months ago

@matklad I usually use sort_unstable_by, but given that our concern is determinism, perhaps that is the wrong move here? However, there are no duplicate keys (it is a HashMap that we are returning as an iterable, after all), so the warnings associated with sort_unstable_by should not apply when sorting by the key element of the tuples? Can you confirm?

matklad commented 10 months ago

unstable sort is fine --- the keys are guaranteed to be unique. We might actually go and pass a custom comparison function, which only compares first elements of the tuples, to reduce the code bloat in a minor way, as we know that comparing the second element will never be necessary.

bzm3r commented 9 months ago

This is now ready for final review, I think!