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

sunfishcode commented 3 years ago

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 commented 3 years ago

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

sunfishcode commented 3 years ago

I just noticed that this PR is very similar to another existing PR, https://github.com/http-rs/http-types/pull/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 commented 3 years ago

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.

sunfishcode commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

Reading this change and https://github.com/http-rs/http-types/pull/296, it looks like the main difference is that https://github.com/http-rs/http-types/pull/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 commented 3 years ago

Also, if we end up merging this one instead of https://github.com/http-rs/http-types/pull/296 , then (assuming you copied the doc comments from https://github.com/http-rs/http-types/pull/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.

sunfishcode commented 3 years ago

@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 :smile: .

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.

joshtriplett commented 3 years ago

@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 :smile:.

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.

http-rs / http-types

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