Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return the environment variables currently known to shell #78

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

bzm3r
Copy link

@bzm3r bzm3r commented Jan 8, 2024

Addresses #77

TO DETERMINE:

  • is it really necessary to have both vars and vars_os?
  • what should the return type of vars/vars_os be? For now, I defaulted to a "minimal iterable which does not retain references to the actual data" (a Vec) (resolved)

src/lib.rs Show resolved Hide resolved
@matklad
Copy link
Owner

matklad commented Jan 8, 2024

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
Copy link
Author

bzm3r commented Jan 8, 2024

@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
Copy link
Owner

matklad commented Jan 9, 2024

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 bzm3r marked this pull request as ready for review January 22, 2024 20:57
@bzm3r
Copy link
Author

bzm3r commented Jan 22, 2024

This is now ready for final review, I think!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants