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 File.empty? #4296

Merged
merged 1 commit into from Nov 15, 2016

Conversation

Projects
None yet
3 participants
@kcdragon
Contributor

kcdragon commented Nov 15, 2016

This PR adds the class methods File.empty? (and File.zero?). I included File.zero? because File.empty? in the docs links to File.zero?. https://ruby-doc.org/core-2.4.0_preview3/File.html#method-c-empty-3F (I'm guessing one is an alias of the other)

See #4293 where feature is originally mentioned.

This PR gets the test test_empty_p in "test/mri/ruby/test_file_exhaustive.rb" to pass. I figured that I didn't need to add any other tests since this functionality is tested by the MRI tests. Let me know if I do need to add more tests.

I did run into an issue while testing with just that file. If I ran just this command

bin/jruby test/mri/ruby/test_file_exhaustive.rb -n test_empty_p

I get an error about missing assert_file.

Error: test_empty_p(TestFileExhaustive):
  NameError: undefined local variable or method `assert_file' for #<TestFileExhaustive:0x29a5f4e7>

I needed to add require_relative 'envutil' (which adds assert_file) in order to run the tests. I would have included that change in this PR but it looks like those tests are generated based on the tests in Ruby and I wasn't sure if changes to those files would be overridden when new tests are brought in. That file is probably eventually required when the whole suite is run.

This is my first time contributing to JRuby so let me know if there is something missing that I need to include.

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

@enebo enebo merged commit e56ded2 into jruby:ruby-2.4 Nov 15, 2016

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
@enebo

This comment has been minimized.

Member

enebo commented Nov 15, 2016

@kcdragon Logic was right on. My only comment for next one is to try and use a full name like filename and not f. We have some other brace logic which is like 95% followed so that might be another style tweak suggestion in the future.

Congratulations you are the on JRuby commit board. The tracks have been greased...onward! :)

@headius

This comment has been minimized.

Member

headius commented Nov 15, 2016

@kcdragon There are some examples of running individual test files from the MRI suite in BUILDING.md. Basically, the test/mri/runner.rb script can be used to set up appropriate paths and such, and there's a feature in there that's used to exclude tests known to fail (for maintaining a high-water mark).

Perhaps we should have a better page in the wiki for this.

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