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

Add guess_mime_type_str #38

Merged
merged 1 commit into from Oct 22, 2018
Merged

Conversation

lnicola
Copy link
Contributor

@lnicola lnicola commented Oct 1, 2018

...that's similar to get_mime_type_str (returns Option<&'static str>) and change guess_mime_type_opt to use it.

@abonander
Copy link
Owner

Sorry for taking so long to look at this.

I think I prefer the guess prefix on the functions that don't return Option; this would be more like mime_str_for_ext (get is kind of redundant I'm just now realizing).

@lnicola
Copy link
Contributor Author

lnicola commented Oct 16, 2018

It does return Option, unless I'm missing something. It should be similar to the other guess functions.

Sorry, I misread that. But...

@lnicola
Copy link
Contributor Author

lnicola commented Oct 16, 2018

So.. Right now, the get_mime_type functions take the extension, while the guess_mime_type ones take a Path. Those without a suffix return a Mime, those with _opt return an Option<Mime>, and the one with with _str return an Option<&'static str>. For symmetry, I'm adding a function that takes a Path and returns Option<&'static str>.

I agree that guess vs. get is a bit confusing (and doesn't exactly scream "extension" vs. "path"), but that's probably for another PR (not to mention a breaking change).

@abonander
Copy link
Owner

Yeah I didn't put a whole load of thought into the namings of any of these functions. I'll probably redo them in 3.0 along with upgrading to mime 1.0 whenever that comes out.

I don't want to add another guess function that returns Option as it's just going to be confusing. The "guess" part is supposed to include returning application/octet-stream as a default assumption, or at least that's the semantics I want to put forward in the future.

@lnicola
Copy link
Contributor Author

lnicola commented Oct 16, 2018

By the way, I don't think it's a good idea to return application/octet-stream when the MIME type is unknown: https://stackoverflow.com/questions/1176022/unknown-file-type-mime/28652339#28652339, broofa/mime#139. It's probably better to not return a MIME type at all.

So we could maybe have mime_{type,str}_for_{path,extension} instead?

@abonander
Copy link
Owner

abonander commented Oct 16, 2018

There's a lot of debate in the comments of the top answer as well as the answer you linked. Those RFCs are specifically about Content-Type headers on HTTP bodies, which falls to the HTTP library the user is using or the user themselves (either of which may or may not choose to delegate to a library like this).

In that case, they should follow the advice of the RFC. In the more general case, however, application/octet-stream is still a safe default. That's why this crate provides functions for both. A less discerning user would just use .unwrap_or("application/octet-stream".parse().unwrap()) anyway and complain about this library not giving them a clean wrapper for it.

@lnicola
Copy link
Contributor Author

lnicola commented Oct 16, 2018

All right; I suppose not everything using MIME is HTTP, so it's fair that providing these helpers might be useful -- although I still think they should be de-emphasised.

{get,guess}_mime_{type,str}_for_{path,ext} would probably be too long, wouldn't it? Or maybe {,guess_}mime_{type,str}_for_{path,ext}?

I can change this PR to use whatever name you think will be in the next version.

@abonander
Copy link
Owner

abonander commented Oct 16, 2018

I still prefer mime_str_for_ext. I'll be dropping get in the future probably, as it doesn't add any information.

If you like you can add notes about assuming application/octet-stream as the default to the docs on the functions that do that. Linking the RFCs from that SO answer wouldn't be a bad idea.

@lnicola
Copy link
Contributor Author

lnicola commented Oct 16, 2018

I still prefer mime_str_for_ext.

This is confusing :-). My function takes a path, not an extension. mime_str_for_path, then?

@abonander
Copy link
Owner

abonander commented Oct 16, 2018

I don't think it's confusing. It's the MIME string for the path's extension. No other part of the path matters. Using "path" in general muddies the interpretation, implies that we might actually load the file to check its structure.

Call it mime_str_for_path_ext if you really want.

@lnicola
Copy link
Contributor Author

lnicola commented Oct 20, 2018

Updated.

@abonander abonander merged commit 48d97bd into abonander:master Oct 22, 2018
@abonander
Copy link
Owner

Looks good, thanks.

@lnicola lnicola deleted the add-guess-str branch October 27, 2018 18:09
@lnicola lnicola mentioned this pull request Jan 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants