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

Add `Dir.empty?` #4301

Merged
merged 1 commit into from Nov 16, 2016

Conversation

Projects
None yet
3 participants
@kcdragon
Contributor

kcdragon commented Nov 16, 2016

This PR adds support for the method Dir.empty? requested in #4293

This 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.

@headius

This comment has been minimized.

Member

headius commented Nov 16, 2016

Looks good!

The MRI tests are hanging right now (I'm currently trying to fix it) so I'll just merge this. Looks fine.

@headius headius added this to the JRuby 9.2.0.0 milestone Nov 16, 2016

@headius headius merged commit 665521d into jruby:ruby-2.4 Nov 16, 2016

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
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();

This comment has been minimized.

@kcdragon

kcdragon Nov 16, 2016

Contributor

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?

This comment has been minimized.

@headius

headius Nov 16, 2016

Member

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.

This comment has been minimized.

@enebo

enebo Nov 16, 2016

Member

I believe this will throw if the directory does not exist. newFileStat creates a FileStat instance and then calls setup which performs the stat.

This comment has been minimized.

@kcdragon

kcdragon Nov 16, 2016

Contributor

@headius @enebo

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);

This comment has been minimized.

@kcdragon

kcdragon Nov 16, 2016

Contributor

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.

@kcdragon

This comment has been minimized.

Contributor

kcdragon commented Nov 16, 2016

@headius Ok cool. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment