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

Change return type of Field::content_type #637

Closed
davidpdrsn opened this issue Dec 17, 2021 · 6 comments · Fixed by #642
Closed

Change return type of Field::content_type #637

davidpdrsn opened this issue Dec 17, 2021 · 6 comments · Fixed by #642
Labels
A-axum C-enhancement Category: A PR with an enhancement E-help-wanted Call for participation: Help is requested to fix this issue.
Milestone

Comments

@davidpdrsn
Copy link
Member

davidpdrsn commented Dec 17, 2021

It should return some kind of string, or a type axum owns, to avoid the public dependency on mime.

Mime implements AsRef<str> meaning we can probably return &str but should verify that it returns the whole mime type and not just a part.

@davidpdrsn davidpdrsn added this to the 0.5 milestone Dec 17, 2021
@davidpdrsn davidpdrsn added A-axum C-enhancement Category: A PR with an enhancement E-help-wanted Call for participation: Help is requested to fix this issue. labels Dec 17, 2021
@nashley
Copy link
Contributor

nashley commented Dec 18, 2021

Why do you prefer a string over mime's enum types? Personally I like being able to match on valid mime types.

It looks like mime has been unmaintained for a while (and will likely continue to be). I'm not sure what a good alternative is though. http-types has mime support, but you probably don't want to pull in that whole library.

@SabrinaJewson
Copy link
Contributor

Why do you prefer a string over mime's enum types?

It says in the issue description - avoiding the public dependency on mime. The purpose is just to avoid having to make breaking changes to Axum should breaking changes be made to mime in future. It would also allow switching Mime implementation without breaking API.

@davidpdrsn
Copy link
Member Author

Yep exactly what @SabrinaJewson said.

@nashley
Copy link
Contributor

nashley commented Dec 19, 2021

Makes sense.

A followup question then: should I expose an enum for mime types within axum, or do you prefer just a string? Either way, I think we'd probably want some (potentially internal) constants for mime types that the server uses like text/plain; charset=utf-8, text/html; charset=utf-8, application/x-www-form-urlencoded, etc. It sounds like you may want to continue to use mime internally, so I'll stick with that.

Perhaps users could be encouraged to parse the strings into enums with other libraries if they so choose? I think that fits into (what I perceive to be) the goal and spirit of axum.

@davidpdrsn
Copy link
Member Author

Yes. We should continue to use mime internally, but expose a string that users can parse into whatever representation is appropriate.

nashley pushed a commit to nashley/axum that referenced this issue Dec 21, 2021
Replaces `Field::content_type`'s return type with `&str`

Closes tokio-rs#637
nashley pushed a commit to nashley/axum that referenced this issue Dec 21, 2021
Replaces `Field::content_type`'s return type with `&str`.
This is a breaking change.

Closes tokio-rs#637
davidpdrsn pushed a commit that referenced this issue Dec 21, 2021
Replaces `Field::content_type`'s return type with `&str`.
This is a breaking change.

Closes #637
@davidpdrsn
Copy link
Member Author

Fixed in #642

davidpdrsn pushed a commit that referenced this issue Dec 27, 2021
Replaces `Field::content_type`'s return type with `&str`.
This is a breaking change.

Closes #637
davidpdrsn pushed a commit that referenced this issue Jan 3, 2022
Replaces `Field::content_type`'s return type with `&str`.
This is a breaking change.

Closes #637
davidpdrsn pushed a commit that referenced this issue Jan 3, 2022
Replaces `Field::content_type`'s return type with `&str`.
This is a breaking change.

Closes #637
davidpdrsn pushed a commit that referenced this issue Jan 23, 2022
Replaces `Field::content_type`'s return type with `&str`.
This is a breaking change.

Closes #637
davidpdrsn pushed a commit that referenced this issue Jan 23, 2022
Replaces `Field::content_type`'s return type with `&str`.
This is a breaking change.

Closes #637
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-axum C-enhancement Category: A PR with an enhancement E-help-wanted Call for participation: Help is requested to fix this issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants