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

File Loop app example #239

Merged
merged 1 commit into from Jul 15, 2017

Conversation

Projects
None yet
2 participants
@Ophirr33
Copy link
Contributor

Ophirr33 commented Jul 14, 2017

This is a rough draft of the file loop example. I'd love some feedback on how to make it more concise. Right now there's a lot of boiler plate on iterating through the file system, and check parent paths. Though, the file system boilerplate could help make a better case for walkdir. @budziq , thanks for the tip on how to make a sym link loop. Fixes #207

@budziq
Copy link
Collaborator

budziq left a comment

Hi @Ophirr33 Thanks for the PR.

As you've discovered you are reimplementing part of walkdir I would suggest to avoid that 😄.

Here are some thoughts:
The original idea was to take a Path:

  • split it into elements and just check each pair without any calls to fs::read_dir or need for a HashMap.
  • please note that PathBuf already acts as a stack implementing pop and push operations
  • I wouldn't worry about performance optimization here. We want to show the general concept and the reader would be perfectly capable of implementing their own caching scheme - so please skip the HashMap

So with this simplified assumptions this could be made into a 4-5 liner.

Also could you:

  • run rustfmt on the code after it's ready
  • squash the commits into one
  • rebase the PR to latest master
src/app.md Outdated
[![same_file-badge]][same_file] [![cat-filesystem-badge]][cat-filesystem]

Use [`same-file::is_same_file`] to detect loops within a directory.
This manually iterates through the filesystem via the [`std::path`] module.

This comment has been minimized.

@budziq

budziq Jul 14, 2017

Collaborator

no need to explicitly mention the std::path module.

@Ophirr33

This comment has been minimized.

Copy link
Contributor Author

Ophirr33 commented Jul 14, 2017

Hey @budziq, thanks for the feedback, I definitely misread what the example was going for.

@budziq
Copy link
Collaborator

budziq left a comment

Yeah lesson for me to be more explicit with descriptions.
Great work.

src/app.md Outdated
let mut path_buf = path.to_path_buf();
while path_buf.pop() {
if is_same_file(&path_buf, path)? {
return Ok(true);

This comment has been minimized.

@budziq

budziq Jul 14, 2017

Collaborator
  • the contains_loop tests only if the whole path is pointing to one of its ancestors while we would like to find all loops. calling it repeatedly on poped path segments will do the trick
  • How about returning the result with optional looped path segments? (this might not come out that clean so I'll leave it up to you)

This comment has been minimized.

@Ophirr33

Ophirr33 Jul 14, 2017

Author Contributor

Not 100% sure if I'm following. Basically it should detect a loop even when you don't specify the looped directory, right?

src/app.md Outdated
// Check this path against all of its parents
fn contains_loop(path: &Path) -> io::Result<bool> {
let mut path_buf = path.to_path_buf();

This comment has been minimized.

@budziq

budziq Jul 14, 2017

Collaborator

we may use AsRef here as well to make it usable with &str

@Ophirr33

This comment has been minimized.

Copy link
Contributor Author

Ophirr33 commented Jul 15, 2017

@budziq It checks all paths and will return what paths it's looping between now. Let me know what else I can do

@budziq
Copy link
Collaborator

budziq left a comment

Excellent. One more tiny detail and we are good to merge

src/app.md Outdated

[![same_file-badge]][same_file] [![cat-filesystem-badge]][cat-filesystem]

Use [`same-file::is_same_file`] to detect loops for a given path.

This comment has been minimized.

@budziq

budziq Jul 15, 2017

Collaborator

Please change the link same-file::is_same_file to same_file::is_same_file

@Ophirr33

This comment has been minimized.

Copy link
Contributor Author

Ophirr33 commented Jul 15, 2017

@budziq Done!

@budziq budziq merged commit fad366b into rust-lang-nursery:master Jul 15, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@budziq

This comment has been minimized.

Copy link
Collaborator

budziq commented Jul 15, 2017

@Ophirr33 Well done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.