Skip to content
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

Properly remove f.name usage in send_file #1988

Merged
merged 3 commits into from
Aug 26, 2016
Merged

Properly remove f.name usage in send_file #1988

merged 3 commits into from
Aug 26, 2016

Conversation

untitaker
Copy link
Contributor

@untitaker untitaker commented Aug 20, 2016

Amend #1849


This change is Reviewable

@untitaker
Copy link
Contributor Author

There is a broken PR in master (#1849) that I've merged, I want somebody else to review this closely.

@untitaker
Copy link
Contributor Author

FWIW, the original discussion is #104 -- I'm torn about mimetype support.

@mitsuhiko
Copy link
Contributor

I'm okay removing etags I suppose but if we stop sending correct mimetypes that's not great.

@untitaker
Copy link
Contributor Author

We don't have to remove anything but usage of f.name attribute I think (as far as I understand this is the only thing that is actually broken design)

@untitaker untitaker changed the title Actually remove etag and mimetype guessing Properly remove f.name usage in send_file Aug 25, 2016
@untitaker
Copy link
Contributor Author

@mitsuhiko Check it out now, a.) Doesn't use f.name b.) Doesn't silently omit MIME-types if none can be guessed but just fails.

@mitsuhiko
Copy link
Contributor

Looks good to me.


Reviewed 1 of 3 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@untitaker
Copy link
Contributor Author

Fixed tests. I wonder if we should still infer the attachment_filename from f.name.

@untitaker
Copy link
Contributor Author

Ah whatever.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants