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 `Pathname#empty?` #4322

Merged
merged 3 commits into from Nov 22, 2016

Conversation

Projects
None yet
2 participants
@kcdragon
Contributor

kcdragon commented Nov 20, 2016

  • Add Pathname#empty?
  • Remove empty? implementation on File since FileTest will assign its implementation of empty? to File (see RubyFileTest.FileTestFileMethods)
  • Add FileTest.empty?

The last two steps were down in this PR in order to base the implementation of Pathname#empty? on the MRI implementation (which uses Dir.empty? and FileTest.empty?. see https://ruby-doc.org/stdlib-2.4.0_preview3/libdoc/pathname/rdoc/Pathname.html#method-i-empty-3F).

This PR gets the failing test in "test/mri/pathname/test_pathname.rb" to pass. The tests in "test/mri/ruby/test_file_exhaustive.rb" are also still passing after the changes to File.

See #4293

Add `Pathname#empty?`
* Add `Pathname#empty?`
* Remove `empty?` implementation on `File` since `FileTest` will assign its implementation of `empty?` to `File` (see `RubyFileTest.FileTestFileMethods`)
* Add `FileTest.empty?`

The last two steps were down in this PR in order to base the implementation of `Pathname#empty?` on the MRI implementation.

See #4293

@enebo enebo added this to the JRuby 9.2.0.0 milestone Nov 21, 2016

@enebo enebo added the core label Nov 21, 2016

@enebo

A bigger change and a smaller change

@@ -444,7 +444,7 @@ public static RubyBoolean writable_p(IRubyObject recv, IRubyObject filename) {
return RubyFileTest.writable_p(recv, filename);
}
@JRubyMethod(name = "zero?", required = 1)
@JRubyMethod(name = {"empty?", "zero?"}, required = 1)
public static RubyBoolean zero_p(ThreadContext context, IRubyObject recv, IRubyObject filename) {
return RubyFileTest.zero_p(context, recv, filename);

This comment has been minimized.

@enebo

enebo Nov 21, 2016

Member

This might work but I can see in MRI source that there is a defintion for zero? in FileTest and one in Dir. Can you add a definition to RubyDir and then remove the directory? dynamic dispatch path. If somehow a Dir does end up calling the FileTest version (like replace Dir#empty? with a def which calls super) then the stat call I think will still work.

This comment has been minimized.

@enebo

enebo Nov 21, 2016

Member

oh another thing to mention is that we try not to add dynamic dispatches when we can help it and this will make ordinary file zero? tests more expensive (due to dyn dispatch and the second stat call).

This comment has been minimized.

@kcdragon

kcdragon Nov 21, 2016

Contributor

@enebo I pushed a new commit that uses direct calls to RubyFileTest and RubyDir instead of callMethod. Is that what you where looking for? I didn't think I needed to "add a definition to RubyDir" because there is already one there for empty? (the method empty_p).

Also, by dynamic dispatch, are you referring to my use of callMethod? That's what I was thinking you were referring to.

Thanks in advance for your feedback. It has been helpful so far.

This comment has been minimized.

@enebo

enebo Nov 21, 2016

Member

@kcdragon shoot. I messed up! I thought your deifnition of zero_p was on RubyFileTest and not on RubyPathname. MRI does do a dynamic call (callMethod or rb_funcall in MRI) in pathname. sorry for messing that up. You had it right the first time :|

This comment has been minimized.

@kcdragon

kcdragon Nov 22, 2016

Contributor

@enebo Ok. No big deal. I undid my most recent changes but kept the change for else style (} else {) in. Let me know if there is anything else.

if (fileTest.callMethod(context, "directory?", getPath()).isTrue()) {
return context.runtime.getDir().callMethod(context, "empty?", getPath());
}
else {

This comment has been minimized.

@enebo

enebo Nov 21, 2016

Member

Our style is to have '} else {' instead of having closing brackets on their own lines.

This comment has been minimized.

@enebo

enebo Nov 21, 2016

Member

DOH. I should have clicked request changes...too fast to click buttons :)

kcdragon added some commits Nov 21, 2016

@enebo enebo merged commit 0a56927 into jruby:ruby-2.4 Nov 22, 2016

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment