-
Notifications
You must be signed in to change notification settings - Fork 106
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
fix downloadable? to account for other file types #3718
Conversation
delegate :basename, :descend, :parent, :join, :to_s, :read, :write, :mkdir, :directory?, :realpath, :pipe?, :readable?, to: :path | ||
delegate :basename, :descend, :parent, :join, :to_s, :read, :write, :mkdir, to: :path | ||
delegate :directory?, :realpath, :readable?, :file?, to: :path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear, this removes pipe?
while adding file?
and putting them on 2 lines.
This looks good but do we want to add a test for this? I didn't see anything for it in the test "cannot download files that are not downloadable?" do
Dir.mktmpdir do |dir|
file = File.join(dir, 'foo.text')
FileUtils.touch(file)
# make sure file is not readable, a condition of PosixFile#downloadable?
FileUtils.chmod(0333, file)
refute PosixFile.new(file).downloadable?
end
end Unless the test is happening somewhere else that I missed. |
@Oglopf Looks like a good test, but I'd change the title of it to |
There's this system test:
|
@johrstrom ah, I missed the systems test. I think that with a unit test in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Fix downloadable? to account for other file types like block devices.
Fix downloadable? to account for other file types like block devices.
fix downloadable? to account for other file types to work on #3026.