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

Issue#63183: Add fs::read_dir() and ReadDir warning about iterator order + example #63356

Merged
merged 5 commits into from
Sep 25, 2019
Merged

Issue#63183: Add fs::read_dir() and ReadDir warning about iterator order + example #63356

merged 5 commits into from
Sep 25, 2019

Conversation

ali-raheem
Copy link
Contributor

As per #63183

Add warning about iterator order to read_dir and ReadDir, add example of explicitly ordering direntrys.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @KodrAus (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 7, 2019
src/libstd/fs.rs Outdated Show resolved Hide resolved
src/libstd/fs.rs Outdated Show resolved Hide resolved
@totsteps
Copy link
Member

Ping from triage, @KodrAus can you please review this.

Thanks.

@KodrAus
Copy link
Contributor

KodrAus commented Aug 22, 2019

Thanks for your patience @ali-raheem! I think this example could be closer to a typical usecase if we retain the DirEntry itself, and sorting by the path:

let mut entries = fs::read_dir(".")?.collect::<Result<Vec<_>, io::Error>>()?;
        
entries.sort_by_key(|e| e.path());

That way the entries collection contains the whole DirEntry along with easy access to other filesystem metadata. It means that we couldn't use the println! statements so easily, but I don't think we really need them.

@abonander
Copy link
Contributor

@KodrAus DirEntry::path() performs a copy, though.

@KodrAus
Copy link
Contributor

KodrAus commented Aug 26, 2019

Ah yes that's not really ideal is it. Thanks @abonander!

Ok I think this will be good to go once we remove the printlns (or convert them into comments).

@ali-raheem
Copy link
Contributor Author

@KodrAus Like this? I've commented out the printlns so if someone wanted they could uncomment them and run it as a demonstration.

@wirelessringo
Copy link

Ping from triage. @KodrAus any updates on this? Thanks.

@KodrAus
Copy link
Contributor

KodrAus commented Sep 8, 2019

Ah sorry @ali-raheem, I meant that I think we should just remove those statements altogether, but could have a comment in the example source like:

fn main() -> io::Result<()> {
    let mut entries = fs::read_dir(".")?
        .map(|res| res.map(|e| e.path()))
        .collect::<Result<Vec<_>, io::Error>>()?;

    // The order in which `read_dir` returns entries is not guaranteed. If reproducible
    // ordering is required the entries should be explicitly sorted.

    entries.sort();

    // The contents of `entries` are now sorted by their path

    Ok(())
}

@abonander
Copy link
Contributor

The entire reason for opening the issue is that there should be a clear statement that the user should not rely on iteration order; comments in examples are too easy to ignore.

@KodrAus
Copy link
Contributor

KodrAus commented Sep 8, 2019

@abonander Hm, I would think prose within the example would be clearer for someone scanning it than println statements they can’t actually see the result of.

@abonander
Copy link
Contributor

Ah, my mistake. I thought you were referring to the statements in the actual doc comments and I just immediately thought "No! That was my whole complaint to begin with!"

@ali-raheem
Copy link
Contributor Author

@KodrAus

Sorry it occured to me you might not get a notification when I push a new commit so I thought I'd ping.

Hopefully made the changes now.

@wirelessringo
Copy link

Ping from triage. @KodrAus any updates on this? Thanks. 😄

Copy link
Contributor

@KodrAus KodrAus left a comment

Choose a reason for hiding this comment

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

Thanks for your patience @ali-raheem!

This looks good to me

@KodrAus
Copy link
Contributor

KodrAus commented Sep 23, 2019

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 23, 2019

📌 Commit 1161aeb has been approved by KodrAus

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 23, 2019
Centril added a commit to Centril/rust that referenced this pull request Sep 23, 2019
Issue#63183: Add fs::read_dir() and ReadDir warning about iterator order + example

As per rust-lang#63183

Add warning about iterator order to read_dir and ReadDir, add example of explicitly ordering direntrys.
Centril added a commit to Centril/rust that referenced this pull request Sep 24, 2019
Issue#63183: Add fs::read_dir() and ReadDir warning about iterator order + example

As per rust-lang#63183

Add warning about iterator order to read_dir and ReadDir, add example of explicitly ordering direntrys.
Centril added a commit to Centril/rust that referenced this pull request Sep 24, 2019
Issue#63183: Add fs::read_dir() and ReadDir warning about iterator order + example

As per rust-lang#63183

Add warning about iterator order to read_dir and ReadDir, add example of explicitly ordering direntrys.
bors added a commit that referenced this pull request Sep 24, 2019
Rollup of 16 pull requests

Successful merges:

 - #63356 (Issue#63183: Add fs::read_dir() and ReadDir warning about iterator order + example)
 - #63934 (Fix coherence checking for impl trait in type aliases)
 - #64016 (Streamline `Compiler`)
 - #64296 (Document the unstable iter_order_by library feature)
 - #64443 (rustdoc: general cleanup)
 - #64622 (Add a cycle detector for generic `Graph`s and `mir::Body`s)
 - #64689 (Refactor macro by example)
 - #64698 (Recover on `const X = 42;` and infer type + Error Stash API)
 - #64702 (Remove unused dependencies)
 - #64717 (update mem::discriminant test to use assert_eq and assert_ne over comparison operators)
 - #64720 ( remove rtp.rs, and move rtpSpawn and RTP_ID_ERROR to libc)
 - #64721 (Fixed issue from #64447)
 - #64725 (fix one typo)
 - #64737 (fix several issues in String docs)
 - #64742 (relnotes: make compatibility section more sterile and fix rustc version)
 - #64748 (Fix #64744. Account for the Zero sub-pattern case.)

Failed merges:

r? @ghost
@bors bors merged commit 1161aeb into rust-lang:master Sep 25, 2019
@ali-raheem ali-raheem deleted the issue#63183 branch September 28, 2019 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants