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

Add Users and Group related functions #1139

Merged
merged 1 commit into from
Nov 1, 2019
Merged

Add Users and Group related functions #1139

merged 1 commit into from
Nov 1, 2019

Conversation

otavio
Copy link
Contributor

@otavio otavio commented Oct 19, 2019

This was a collaborative work between Johannes Schilling
dario@deaktualisierung.org, Fredrick Brennan copypaste@kittens.ph
and myself.

This PR is a split-off from #864 so we merge ready parts first. Next, we work on the iterators.

Signed-off-by: Otavio Salvador otavio@ossystems.com.br

@asomers
Copy link
Member

asomers commented Oct 20, 2019

So what's the plan? What do you expect to add in a follow up PR?

@asomers asomers mentioned this pull request Oct 20, 2019
@otavio
Copy link
Contributor Author

otavio commented Oct 21, 2019

@asomers we did not include the iterators here and I'd like to extend it after this is merged as it is independent and allow for reducing the code to work/review.

Do you mind to go over this here so we clean any leftover and merge this one?

@otavio
Copy link
Contributor Author

otavio commented Oct 27, 2019

@asomers any comment on this one?

Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I'm almost inclined to merge as-is, except for the one issue I raised inline. Also, it needs a CHANGELOG entry.

src/unistd.rs Outdated Show resolved Hide resolved
src/unistd.rs Outdated Show resolved Hide resolved
@otavio
Copy link
Contributor Author

otavio commented Oct 30, 2019

@asomers I made the changes. Please let me know if something else is still needed.

@asomers
Copy link
Member

asomers commented Oct 30, 2019

Now that you no longer Errno::clear, you need to remove some unsafe blocks, apparently.

This was a collaborative work between Johannes Schilling
<dario@deaktualisierung.org>, Fredrick Brennan <copypaste@kittens.ph>
and myself.

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
@otavio
Copy link
Contributor Author

otavio commented Oct 30, 2019

Now that you no longer Errno::clear, you need to remove some unsafe blocks, apparently.

Just did; sorry for missing that.

@otavio
Copy link
Contributor Author

otavio commented Oct 31, 2019

@asomers fixed the unnecessary unsafe block. Should be good to go.

otavio added a commit to ctrlcctrlv/nix that referenced this pull request Oct 31, 2019
This was a collaborative work between Johannes Schilling
<dario@deaktualisierung.org>, Fredrick Brennan <copypaste@kittens.ph>
and myself.

This is a continuation of nix-rust#1139.

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bors r+

bors bot added a commit that referenced this pull request Nov 1, 2019
1139: Add `Users` and `Group` related functions r=asomers a=otavio

This was a collaborative work between Johannes Schilling
<dario@deaktualisierung.org>, Fredrick Brennan <copypaste@kittens.ph>
and myself.

This PR is a split-off from #864 so we merge ready parts first. Next, we work on the iterators.

Signed-off-by: Otavio Salvador <otavio@ossystems.com.br>

Co-authored-by: Otavio Salvador <otavio@ossystems.com.br>
@bors
Copy link
Contributor

bors bot commented Nov 1, 2019

Build succeeded

@bors bors bot merged commit b14415a into nix-rust:master Nov 1, 2019
@pskopnik pskopnik mentioned this pull request Aug 16, 2022
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.

2 participants