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

Implement walkdir. fixes #8814 #13707

Merged
merged 1 commit into from
Oct 24, 2015
Merged

Implement walkdir. fixes #8814 #13707

merged 1 commit into from
Oct 24, 2015

Conversation

dhoegh
Copy link
Contributor

@dhoegh dhoegh commented Oct 21, 2015

This implements a walkdir function equivalent to Python's os.walk. I have added test for the function and it's keyword arguments. Please correct the docs, as I am not a native speaker. My only remaining question is whether walkdir should do as os.walk and swallow errors as default.
Fixes #8814

@johnmyleswhite
Copy link
Member

Years after we first argued about adding a function to do this, I hope we can add this one.

walkdir(dir; topdown=true, follow_symlinks=false, onerror=throw)

The walkdir method return an iterator that walks the directory tree of a directory. The iterator returns a tuple containing
`(rootpath, dirs, files)`. The directory tree can be transversed top-down and bottom-up. If walkdir encounters a SystemError
Copy link
Contributor

Choose a reason for hiding this comment

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

traversed top-down or bottom-up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@dhoegh
Copy link
Contributor Author

dhoegh commented Oct 22, 2015

@tkelman I have addressed the comments and force pushed a new version.

walkdir(dir; topdown=true, follow_symlinks=false, onerror=throw)

The walkdir method return an iterator that walks the directory tree of a directory. The iterator returns a tuple containing
`(rootpath, dirs, files)`. The directory tree can be transversed top-down or bottom-up. If walkdir encounters a SystemError
Copy link
Contributor

Choose a reason for hiding this comment

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

missed a typo, traversed not transversed

@tkelman
Copy link
Contributor

tkelman commented Oct 22, 2015

This is really nicely done and well tested, so +1 from me. It'll either have to be exported from Base or qualified in the tests. If it gets exported, then a signature for it should be added in the RST stdlib docs so doc/genstdlib.jl can populate the docstring.

@dhoegh dhoegh force-pushed the walkdir branch 2 times, most recently from 504281b to af2f650 Compare October 22, 2015 05:29
@dhoegh
Copy link
Contributor Author

dhoegh commented Oct 22, 2015

If it gets exported, then a signature for it should be added in the RST stdlib docs so doc/genstdlib.jl can populate the docstring.

I have now exported it from Base. How should I add it to RST stdlib docs, any good examples?

@tkelman
Copy link
Contributor

tkelman commented Oct 22, 2015

The process is described at https://github.com/JuliaLang/julia/blob/master/CONTRIBUTING.md#improving-documentation, sorry it's slightly messy at the moment.

Take a look at https://raw.githubusercontent.com/JuliaLang/julia/master/doc/stdlib/file.rst, I think what you have to do is populate just the first .. function:: line with your new signature, then run make && usr/bin/julia doc/genstdlib.jl && git diff and check that you get your desired docstring under a new .. Docstring generated from Julia source

@dhoegh
Copy link
Contributor Author

dhoegh commented Oct 22, 2015

Ok. I have possibly found a good way to test the onerror keyword argument, I will continue the work on the PR later today European time:)

@dhoegh dhoegh force-pushed the walkdir branch 2 times, most recently from eeba237 to a9780eb Compare October 22, 2015 19:25
@dhoegh
Copy link
Contributor Author

dhoegh commented Oct 22, 2015

I have added test for the onerror keyword and the tests should pass now. There were some name conflicts and platform specific issues. The only thing missing is docs and decide if the function should swallow the errors as default as python.

My Ubuntu cannot build due to an openssl error while building dependencies. I will see if I can figure it out, but it will not be before Sunday.

@tkelman
Copy link
Contributor

tkelman commented Oct 22, 2015

apt-get install libssl-dev

What's the justification for ignoring errors by default? What kind of scenarios would cause errors? I don't like the sound of ignoring errors by default, but how often would errors be expected?

@test files == ["file_dir2"]

end
@unix_only rm(dirwalk, recursive=true)
Copy link
Contributor

Choose a reason for hiding this comment

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

can't merge like this, cleanup is necessary on windows too

@hayd
Copy link
Member

hayd commented Oct 23, 2015

What's the justification for ignoring errors by default?

The default is to throw all errors, if you pass a different function to onerrors it applies that to the error (if it's a SystemError), if it's not a SystemError it raises no matter what.

I'm not a huge fan of that API however, I think a Boolean (or even a String) to throw/suppress errors would be better than passing around a function - that's the python API but it seems a little un-julian.

@dhoegh
Copy link
Contributor Author

dhoegh commented Oct 23, 2015

apt-get install libssl-dev

The sudo apt-get install libssl-dev fails due to a server problem see: https://gist.github.com/dhoegh/562c47d1370ec62ed69b. I have tried sudo apt-get update without any luck.

I don't like the sound of ignoring errors by default, but how often would errors be expected?

Errors will occur if the user move directories or change the current working directory while using a relative path to the root directory while iterating walkdir. I think the python reasoning is that just because one directory fails it could still walk the rest of a large directory tree. I do also prefer it to throw an error as default, but I thought we at least should consider how Python does it.

I'm not a huge fan of that API however, I think a Boolean (or even a String) to throw/suppress errors would be better than passing around a function - that's the python API but it seems a little un-julian.

The API is made with a function because then the user could make some kind of handling of the missing dir. Python's OSError provide the path to the file which it failed to read and then the user could inspect the failure more closely with a function. Julia's SystemError do unfortunately not provide a direct way of getting the path for the file it failed to read. I have no problem to change it to a Boolean flag as I have never used the onerror keyword but it will not be as versatile if people wan't some special handling.

edit:
I have changed the test to use touch and made windows cleanup.

@tkelman
Copy link
Contributor

tkelman commented Oct 23, 2015

Utopic is 14.10, right? I think that went end-of-life a few months ago: http://fridge.ubuntu.com/2015/07/03/ubuntu-14-10-utopic-unicorn-reaches-end-of-life-on-july-23-2015/ so you probably need to dist-upgrade.

The handling callback seems okay to me and more flexible than a boolean flag. Though moving files around during walkdir and wanting special error handling seems like it might be a fairly niche thing to do, where worst-case you could reimplement your own version of walkdir in a few lines.

@dhoegh
Copy link
Contributor Author

dhoegh commented Oct 24, 2015

I have executed julia doc/genstdlib.jl, committed the changes to file.rst and squashed the commits. Ping me if anything needs to be changed before a merge.

@hayd
Copy link
Member

hayd commented Oct 24, 2015

30,000th travis build passed!

hayd added a commit that referenced this pull request Oct 24, 2015
@hayd hayd merged commit 9843a88 into JuliaLang:master Oct 24, 2015
@dhoegh dhoegh deleted the walkdir branch October 24, 2015 18:12
@parpwhick
Copy link
Contributor

This seems useful also for 0.4, is there any chance of a backport?

@tkelman
Copy link
Contributor

tkelman commented Oct 26, 2015

I'd rather avoid backporting new features in most cases, especially not without being proven on master for some time. This is a pretty small amount of pure julia code and could easily be put in a package, even Compat.jl, for use on 0.4 and even 0.3.

@StefanKarpinski
Copy link
Member

Putting it in Compat is the right way to go. As a policy, we don't backport features.

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.

6 participants