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.

Sign up for GitHub

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

Jump to bottom

Dir: Implement IntoIterator for Dir #1333

Merged
merged 1 commit into from
Feb 15, 2021
Merged

Dir: Implement IntoIterator for Dir #1333

merged 1 commit into from
Feb 15, 2021

Conversation

wmanley
Copy link
Contributor

@wmanley wmanley commented Nov 15, 2020

This is useful to allow returning an iterator based on a directory iterator
without needing a self-referential struct.

@asomers
Copy link
Member

asomers commented Nov 28, 2020

I don't understand the motivation for this PR. Can you please show an example? What does this enable that couldn't as easily be done with the existing API?

@wmanley
Copy link
Contributor Author

wmanley commented Nov 29, 2020

Here's where I use it: https://github.com/wmanley/ostree-oxide/blob/d3f38da61deb614be67d3705519df0235e4d2e84/ostree-repo/src/lib.rs#L253-L281

This allows me to return an Iterator that owns the Dir. This is important because I open the Dir in the function that returns the Iterator. I couldn't work out how to do this with the existing API short of self-referential structs.

@asomers
Copy link
Member

asomers commented Nov 29, 2020

Would it be possible to add a usage example?

@wmanley
Copy link
Contributor Author

wmanley commented Nov 29, 2020 •
edited
Loading

Would it be possible to add a usage example?

Done, it's a little contrived, but illustrative.

@wmanley
Copy link
Contributor Author

wmanley commented Feb 11, 2021 •
edited
Loading

@asomers Ping: Are there any other changes you'd like to make this suitable to go in?

asomers
asomers requested changes Feb 13, 2021
View reviewed changes
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.

I think this looks good. It just needs three small things:

  1. CHANGELOG entry (may require a rebase to avoid conflicts)
  2. Squash
  3. Doc comment for OwningIter. It can simply say "The return type of [Dir::into_iter]".

@wmanley
Dir: Implement IntoIterator for Dir
91c2a08 
This is useful to allow returning an iterator based on a directory iterator
without needing a self-referential struct.
@wmanley
Copy link
Contributor Author

wmanley commented Feb 15, 2021

I think this looks good. It just needs three small things:

Thanks for the review. I've done these three things.

CI on "OSX x86_64" failed with:

warning Agent process on a persistent worker exited unexpectedly!

but I can't imagine that the failure was caused by this PR.

asomers
asomers approved these changes Feb 15, 2021
View reviewed changes
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.

The OSX failure looks like perhaps GCE yanked the VM out from under us. That happens sometimes.

bors r+

@bors
Copy link
Contributor

bors bot commented Feb 15, 2021

Build succeeded:

@bors bors bot merged commit de43d76 into nix-rust:master Feb 15, 2021
@wmanley wmanley deleted the dir_OwningIter branch February 15, 2021 15:58
This was referenced Mar 12, 2021
Merged
Closed
Closed
wmanley added a commit to ostreedev/ostree-oxide that referenced this pull request May 10, 2022
@wmanley
ostree-fuse: Update nix dependency to 0.20
b408734 
My [patch to nix](nix-rust/nix#1333) was merged and
now it's upstream.  This was causing builds to fail because I deleted the
repo from `wmanley/nix` after merging.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asomers asomers asomers approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@wmanley @asomers