UploadedFile: Handle content being a Pathname #210
Conversation
|
Great job @perlun ! |
|
(For reference, two people have written in #207 that the fix works for them. 🎉) |
lib/rack/test/uploaded_file.rb
Outdated
| def initialize(content, content_type = 'text/plain', binary = false, original_filename: nil) | ||
| if content.respond_to?(:read) | ||
| if content.respond_to?(:read) && !content.is_a?(Pathname) |
There was a problem hiding this comment.
if content.respond_to?(:read) && (content.is_a?(IO) || content.is_a?(StringIO)) is more safety code in case of that we added new class for content?
There was a problem hiding this comment.
Good suggestion! Will apply this idea.
| @@ -18,8 +19,16 @@ class UploadedFile | |||
| # The content type of the "uploaded" file | |||
| attr_accessor :content_type | |||
|
|
|||
| # Creates a new UploadedFile instance. | |||
| # | |||
| # @param content [IO, Pathname, String, StringIO] a path to a file, or an {IO} or {StringIO} object representing the | |||
There was a problem hiding this comment.
@param content [IO, Pathname, String, StringIO] a path to a file
Is this correct?
For example, IO is used for both a path to a file and file object, right?
There was a problem hiding this comment.
Pathname does not derive from IO, or how do you mean?
I think we go with the "later" approach for now. But feel free to add them if you like, and have time & energy for it. |
Dynamic languages... Can't live with them, can't live without them. In #149, we added support for `UploadedFile` being an `StringIO `object, by means of the `if content.respond_to?(:read)` logic. _However_, that had the unintended consequence of breaking the use case when a `Pathname` is provided instead of a string as the `content` paramer, since a `Pathname` _also_ responds to the `read` message... This PR works around that, by adding some extra checking. As an added bonus, I also added some YARD comments to make it more clear what parameter types this method expects/accepts. Fixes #207.
perlun
force-pushed
the
fix/uploaded-file-regression
branch
from
f522560
to
bf79482
Compare
|
@perlun Thanks for the releasing! Of course I am fine for the "later" approach. |
|
@> Of course I am fine for the "later" approach.
That's perfectly fine with me. We're all human. |
In #210, we tried to fix this but the fix was perhaps not really good enough... So let's relax this check a bit and hopefully unbreak these use cases.
In #210, we tried to fix this but the fix was perhaps not really good enough... So let's relax this check a bit and hopefully unbreak these use cases.
* UploadedFile: Handle content being a Pathname Dynamic languages... Can't live with them, can't live without them. In rack#149, we added support for `UploadedFile` being an `StringIO `object, by means of the `if content.respond_to?(:read)` logic. _However_, that had the unintended consequence of breaking the use case when a `Pathname` is provided instead of a string as the `content` paramer, since a `Pathname` _also_ responds to the `read` message... This PR works around that, by adding some extra checking. As an added bonus, I also added some YARD comments to make it more clear what parameter types this method expects/accepts. Fixes rack#207. * Peer suggestion: Invert check to make the code safer.
In rack#210, we tried to fix this but the fix was perhaps not really good enough... So let's relax this check a bit and hopefully unbreak these use cases.
Dynamic languages... Can't live with them, can't live without them.
In #149, we added support for
UploadedFilebeing anStringIOobject, by means of theif content.respond_to?(:read)logic. However, that had the unintended consequence of breaking the use case when aPathnameis provided instead of a string as thecontentparamer, since aPathnamealso responds to thereadmessage...This PR works around that, by adding some extra checking. As an added bonus, I also added some YARD comments to make it more clear what parameter types this method expects/accepts.
Fixes #207.