Add Dir.empty?
#4301
Add Dir.empty?
#4301
Conversation
|
Looks good! The MRI tests are hanging right now (I'm currently trying to fix it) so I'll just merge this. Looks fine. |
| Ruby runtime = context.runtime; | ||
| RubyString path = StringSupport.checkEmbeddedNulls(runtime, RubyFile.get_path(context, arg)); | ||
| RubyFileStat fileStat = runtime.newFileStat(path.asJavaString(), false); | ||
| boolean isDirectory = fileStat.directory_p().isTrue(); |
There was a problem hiding this comment.
This logic is taken from the exist method below. However, unlike that method, I want the exception to be thrown since empty? should throw an error if the path does not refer to an actual path. This is covered by the test assert_raise(Errno::ENOENT) {Dir.empty?(@nodir)}
Maybe there is a more expressive way to do this?
There was a problem hiding this comment.
Oh, I wasn't aware you had additional changes planned.
So the problem is that this isn't raising now, yes? You can just raise the error yourself via throw runtime.newErrnoENOENT(....). If there's common logic across a couple methods, that could also be pulled out into something common.
There was a problem hiding this comment.
I believe this will throw if the directory does not exist. newFileStat creates a FileStat instance and then calls setup which performs the stat.
There was a problem hiding this comment.
I didn't have any changes planned. The code does throw a Errno::ENOENT in the correct scenario. My only concern was that I was relying on a side effect of boolean isDirectory = fileStat.directory_p().isTrue(); to throw that error. This is probably fine, but I thought I should point it out since it could be unclear to future developers since I don't explicitly throw the error in this method.
| RubyString path = StringSupport.checkEmbeddedNulls(runtime, RubyFile.get_path(context, arg)); | ||
| RubyFileStat fileStat = runtime.newFileStat(path.asJavaString(), false); | ||
| boolean isDirectory = fileStat.directory_p().isTrue(); | ||
| return runtime.newBoolean(isDirectory && entries19(context, recv, arg).getLength() <= 2); |
There was a problem hiding this comment.
The check for <= 2 looks a little odd, but that is because entries19 includes the paths "." and "..". I couldn't find functionality that provides the "real" paths in the directory.
Let me know if you know of a clearer way to do this check.
|
@headius Ok cool. Thanks! |
This PR adds support for the method
Dir.empty?requested in #4293This PR gets the test
test_empty?in "test/mri/ruby/test_dir.rb" passing.There are a couple lines I want to point out that I will do so in comments.