Rename Body::from_file to Body::from_path, add Body::from_file, handle filename-based MIME-type guessing for File objects by sunfishcode · Pull Request #318 · http-rs/http-types

sunfishcode · 2021-01-15T23:25:44Z

This splits a Body::from_open_file out from Body::from_file so that
users who want to use a file opened in a manner other than File::open
can do so.

In particular, this will help users using cap_std::fs::Dir::open to
open files without requiring http-types to have a dependency on cap_std.

sunfishcode · 2021-01-15T23:38:38Z

The clippy error doesn't look related to this patch.

sunfishcode · 2021-01-16T04:13:26Z

I just noticed that this PR is very similar to another existing PR, #296.

The main difference is that this PR's File constructor also takes a Path for Mime type detection (it may be empty if no path is available).

Also, this PR uses different names: from_file and from_open_file instead of from_path and from_file. Either way seems fine to me. The names in this PR are semver-compatible, for what that's worth.

Fishrock123 · 2021-01-19T19:30:15Z

I prefer #296. from_file should take an already open file. It is probably the developer's responsibility to then ensure an appropriate mime fallback exists if magic byte sniffing doesn't work out.

Fishrock123

As per my former comment

sunfishcode · 2021-01-19T21:41:10Z

I'm ok with from_file taking an already open file.

Concerning the mime type, there are some situations where mime_guess will positively identify a file as application/octet-stream, in which case checking for mime::BYTE_STREAM and overriding it with a fallback would produce different results. Consequently, I'd prefer to keep the path argument.

sunfishcode · 2021-01-21T00:55:24Z

I've now updated this PR to use the from_file/from_path renaming. And rebased it to pick up the clippy fix on main, so the CI is now green. As mentioned above, I preserved the path argument in from_file.

yoshuawuyts · 2021-01-22T17:24:49Z

If I'm not mistaken this appears to be a breaking change. I'm okay with merging this, but we'll have to do this as part of our 3.0 effort. Luckily that should be kicking off tomorrow (last pre-3.0 http-types release today), so we'll be able to merge this soon! (:

joshtriplett · 2021-01-23T22:44:51Z

src/body.rs

+    /// to `application/octet-stream`.
+    ///
+    /// The path here is only used to provide an extension for guessing the Mime
+    /// type, and may be empty if the path is unknown.


I think I'd prefer Option<&std::path::Path> rather than requiring the caller to construct and pass an empty Path.

joshtriplett · 2021-01-23T22:57:26Z

Reading this change and #296, it looks like the main difference is that #296 only tries to guess the MIME type in from_path, while this change makes from_file take a path that can be used for extension-based MIME-type guessing.

I made one comment above about the path argument (using Option).

I personally think the common case of from_file will not be to pass a path, and I don't think we should make most callers of from_file pass that argument. I would propose one of two options, depending on how common you think this is:

Make this an internal function from_file_with_path, and have the public from_file pass None. This would require callers to specify the MIME type themselves. Callers can call Mime::from_extension if they want to do filename-based MIME-type guessing. This seems like the right solution if we don't expect it to be especially common to do filename-based MIME-type guessing for an already-open File.
Make this a public function from_file_with_path, and have a public from_file that doesn't take a path. This would allow callers to call from_file_from_path in the case where they want filename-based MIME-type guessing, or from_file if they don't. This seems like the right solution if we expect it to be especially common to do filename-based MIME-type guessing for an already-open File.

Personally, I suspect it won't be especially common to pass an already-open File but still want filename-based MIME-type guessing, so I would propose that we go with the first option.

Does that seem reasonable to you, @sunfishcode?

joshtriplett · 2021-01-23T23:04:10Z

Also, if we end up merging this one instead of #296 , then (assuming you copied the doc comments from #296 ) could you please add a Co-authored-by to that one commit? We can't do anything about the duplicate efforts at this point, but I want to make sure folks get appropriate credit.

This splits a `Body::from_open_file` out from `Body::from_file` so that users who want to use a file opened in a manner other than `File::open` can do so. In particular, this will help users using [`cap_std::fs::Dir::open`] to open files without requiring http-types to have a dependency on `cap_std`. [`cap_std::fs::Dir::open`]: https://docs.rs/cap-std/latest/cap_std/fs/struct.Dir.html#method.open Co-authored-by: brightly-salty <github.downland@aleeas.com>

…no path.

sunfishcode · 2021-01-24T01:17:14Z

@joshtriplett The from_file_with_path option works for me. I don't know how common it will be to pass a path, but my own use case does want this 😄 .

The doc comment wasn't copied from #296; it's likely that we both copied the comment from the pre-existing from_file function and made similar obvious changes. That said, I've now added Co-authored-by, to reflect that we both did similar work here.

Implemented via subsequent changes.

joshtriplett · 2021-01-24T05:16:15Z

@sunfishcode wrote:

@joshtriplett The from_file_with_path option works for me. I don't know how common it will be to pass a path, but my own use case does want this 😄.

Fair enough.

The doc comment wasn't copied from #296; it's likely that we both copied the comment from the pre-existing from_file function and made similar obvious changes.

Ah, makes sense.

That said, I've now added Co-authored-by, to reflect that we both did similar work here.

Thank you for your graceful handling of this. I know that it's frustrating when work gets duplicated.

Merging.

yoshuawuyts mentioned this pull request Jan 18, 2021

Fix clippy lints #321

Merged

Fishrock123 previously requested changes Jan 19, 2021

View reviewed changes

yoshuawuyts added the semver-major This change requires a semver major change label Jan 22, 2021

joshtriplett reviewed Jan 23, 2021

View reviewed changes

joshtriplett changed the title ~~Add a Body::from_open_file constructor.~~ Rename Body::from_file to Body::from_path, add Body::from_file, handle filename-based MIME-type guessing for File objects Jan 23, 2021

This was referenced Jan 23, 2021

Rename Body::from_file, add Body::from_file #296

Closed

Body::from_file should actually be Body::from_path, and Body::from_file should take a File #185

Closed

sunfishcode and others added 4 commits January 23, 2021 17:10

Rename from_file to from_path and from_open_file to from_file.

176deda

Update tests.

08bb900

Rename from_file to from_file_with_path and add from_file with …

096c24c

…no path.

joshtriplett merged commit bf5ace6 into http-rs:main Jan 24, 2021

Fishrock123 mentioned this pull request Apr 29, 2021

Add specialized body types to RequestBuilder http-rs/surf#302

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename Body::from_file to Body::from_path, add Body::from_file, handle filename-based MIME-type guessing for File objects#318

Rename Body::from_file to Body::from_path, add Body::from_file, handle filename-based MIME-type guessing for File objects#318
joshtriplett merged 4 commits intohttp-rs:mainfrom
sunfishcode:main

sunfishcode commented Jan 15, 2021

Uh oh!

sunfishcode commented Jan 15, 2021

Uh oh!

sunfishcode commented Jan 16, 2021

Uh oh!

Fishrock123 commented Jan 19, 2021

Uh oh!

Fishrock123 left a comment

Uh oh!

sunfishcode commented Jan 19, 2021

Uh oh!

sunfishcode commented Jan 21, 2021

Uh oh!

yoshuawuyts commented Jan 22, 2021

Uh oh!

joshtriplett Jan 23, 2021

Uh oh!

joshtriplett commented Jan 23, 2021

Uh oh!

joshtriplett commented Jan 23, 2021

Uh oh!

sunfishcode commented Jan 24, 2021

Uh oh!

joshtriplett commented Jan 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

sunfishcode commented Jan 15, 2021

Uh oh!

sunfishcode commented Jan 15, 2021

Uh oh!

sunfishcode commented Jan 16, 2021

Uh oh!

Fishrock123 commented Jan 19, 2021

Uh oh!

Fishrock123 left a comment

Choose a reason for hiding this comment

Uh oh!

sunfishcode commented Jan 19, 2021

Uh oh!

sunfishcode commented Jan 21, 2021

Uh oh!

yoshuawuyts commented Jan 22, 2021

Uh oh!

joshtriplett Jan 23, 2021

Choose a reason for hiding this comment

Uh oh!

joshtriplett commented Jan 23, 2021

Uh oh!

joshtriplett commented Jan 23, 2021

Uh oh!

sunfishcode commented Jan 24, 2021

Uh oh!

joshtriplett commented Jan 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants