-
-
Notifications
You must be signed in to change notification settings - Fork 930
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
Make the file argument to UploadFile required #1413
Changes from all commits
b621cae
fab8d41
d5d339f
ab97325
3bf4d15
a7b8e05
315b292
428ba3b
b509fa1
c012aa1
3a75692
01c781c
1b9568e
655c4be
e01ecef
4cbb66f
2a312a8
0982127
0bea64d
e5320e5
1c13abb
6f6b3a0
6f608e1
1e51edd
75ca723
18213b5
419d8aa
7a39f30
1a8f11c
4a1325e
bece2af
2b12b51
9b2cb49
fa11ecb
76519cd
1baf994
1fb8b08
b959261
f053b8d
d84a7d5
d9cfaa2
8babda3
5d796bb
9831c89
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,3 @@ | ||
import tempfile | ||
import typing | ||
from collections.abc import Sequence | ||
from shlex import shlex | ||
|
@@ -428,28 +427,24 @@ class UploadFile: | |
An uploaded file included as part of the request data. | ||
""" | ||
|
||
spool_max_size = 1024 * 1024 | ||
file: typing.BinaryIO | ||
headers: "Headers" | ||
|
||
def __init__( | ||
self, | ||
filename: str, | ||
file: typing.Optional[typing.BinaryIO] = None, | ||
content_type: str = "", | ||
file: typing.BinaryIO, | ||
*, | ||
filename: typing.Optional[str] = None, | ||
headers: "typing.Optional[Headers]" = None, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given that we're changing the (internal-ish) interface at this point, should we switch it around so that def __init__(
self,
file: typing.Optional[typing.BinaryIO],
*,
filename: str = "",
content_type: str = "",
headers: "typing.Optional[Headers]" = None,
):
self.file = file
self.filename = filename or getattr(file, 'name', '')
self.content_type = content_type
self.headers = headers There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep makes sense. How do you feel about: def __init__(
self,
file: typing.Optional[typing.BinaryIO],
*,
filename: typing.Optional[str] = None,
content_type: typing.Optional[str] = None,
headers: typing.Optional[Headers] = None,
):
... That way you can differentiate between a filename or content-type that was an empty string or one that was missing altogether (I know
Hmm about this. I feel like it could lead to confusion. I would rather There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I think it would be best to just make it a constructor parameter and not fall back to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, do we want They're derived from the headers, so why not switch to... class UploadFile:
def __init__(
self,
file: typing.Optional[typing.BinaryIO],
*,
headers: typing.Optional[Headers] = None,
):
... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My vote would be to make The main reason for this is a practical one: extracting the filename from the headers requires knowledge that the request was originally a multipart/form-data request (that is, as far as I know, the only way a client would include that in the Also not every implementer of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay - that's not unreasonable. The one other practical case where I could see Eg. In that kinda use-case it's useful having So yeah, sounds good to me. 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep that is a use case I personally really want: https://github.com/adriangb/xpresso/blob/c4455b29d323df1c59808ccafb306b9e7bbd80d1/xpresso/binders/_extractors/body/file.py#L40 I'm glad you brought it up and not me, I didn't want to introduce my biases 😅 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI I already pushed these changes / implementation @tomchristie, so I think this is ready for one last round of review for any typos/nits and then maybe a ✅ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi all. It would be nice if the release could include the removal of |
||
) -> None: | ||
self.filename = filename | ||
self.content_type = content_type | ||
if file is None: | ||
self.file = tempfile.SpooledTemporaryFile(max_size=self.spool_max_size) # type: ignore[assignment] # noqa: E501 | ||
else: | ||
self.file = file | ||
self.file = file | ||
self.headers = headers or Headers() | ||
|
||
@property | ||
def content_type(self) -> typing.Optional[str]: | ||
return self.headers.get("content-type", None) | ||
adriangb marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
@property | ||
def _in_memory(self) -> bool: | ||
# check for SpooledTemporaryFile._rolled | ||
rolled_to_disk = getattr(self.file, "_rolled", True) | ||
return not rolled_to_disk | ||
|
||
|
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.
We could also make this strictly
file: SpooledTemporaryFile
since that's the only way it's ever usedThere 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.
typing.BinaryIO
makes more sense to me.It's an implementation detail that we happen to use
SpooledTemporaryFile
.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.
Typing-wise, isn't there a better type for this situation? Also... It's not an implementation detail, it's written on our docs that the file is a
SpooledTemporaryFile
.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.
Right now there's three options:
IO[bytes]
SpooledTemporaryFile
directlySource: python/typing#829 (comment)
I don't think we should choose (2) and I think we should change our docs. Let's not lock ourselves into concrete implementations if we don't need to.
I think (1) has issues (as mentioned in that discussion and others) but is widely used so it's pretty defensible if a user had to add a
# type: ignore
to that parameter if they pass in some less common file-like object.(3) is the correct option and would look like:
I think the main con here is not the LOC or complexity, the main con is that if we want to start using other methods/attributes of
IO
in the future it would technically be a breaking change. But maybe that's a good thing?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.
That's enough for me. 👍