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 MIME::MediaType #7077

Merged

Conversation

straight-shoota
Copy link
Member

@straight-shoota straight-shoota commented Nov 13, 2018

This PR adds a MIME::MediaType type with methods for parsing mime media types as per RFC 2045 and RFC 2046 including support for optional parameters.

This type is put to use for reading and writing HTTP Content-Type headers and HTTP::Multipart boundaries replacing existing incomplete implementations.

This is not directly integrated with the MIME registry (added in #5765) but can be optionally used together.

Closes #5686

@straight-shoota straight-shoota force-pushed the jm/feature/mime-media-type branch from fd7b793 to e01f1aa Compare November 14, 2018 10:18
Copy link
Member

@RX14 RX14 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs code comments on the "tricky bits", especially the rfc2231 handling

@@ -5,8 +5,7 @@ require "mime"
module MIME
# A MediaType describes a MIME content type with optional parameters.
struct MediaType
property media_type : String
property params : Hash(String, String)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? Having access to raw params Hash could come handy, when removed there's no way to do that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MediaType directly exposes #[], #[]?, #fetch, #[]= and #each_parameter. Why would yo need direct access to params?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For serialization as an example.

Copy link
Member Author

@straight-shoota straight-shoota Nov 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What kind of serialization? I figure you'd either serialize with #to_s or directly access the ivar @params.

@straight-shoota straight-shoota force-pushed the jm/feature/mime-media-type branch from ffb4b52 to da6ce50 Compare November 26, 2018 14:13
@straight-shoota straight-shoota force-pushed the jm/feature/mime-media-type branch from 90de96c to 92d2b18 Compare December 5, 2018 14:37
@straight-shoota
Copy link
Member Author

@RX14 I removed sort from #to_s without an different implementation for #inspect. This can be improved later, if necessary.

@straight-shoota straight-shoota force-pushed the jm/feature/mime-media-type branch from 92d2b18 to 3663040 Compare December 6, 2018 18:08
Copy link
Member

@RX14 RX14 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd still like many more code comments to explain the implicit state while parsing. Especially the rfc2231 stuff.

Copy link
Member

@sdogruyol sdogruyol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally, thank you @straight-shoota 👍

@RX14 RX14 added this to the 0.27.1 milestone Dec 20, 2018
@RX14
Copy link
Member

RX14 commented Dec 20, 2018

Squash please!

@straight-shoota straight-shoota force-pushed the jm/feature/mime-media-type branch 2 times, most recently from c083d5f to 623ffca Compare December 22, 2018 13:09
@straight-shoota straight-shoota force-pushed the jm/feature/mime-media-type branch from 623ffca to f4f1151 Compare December 22, 2018 15:28
@sdogruyol sdogruyol merged commit fc4360b into crystal-lang:master Jan 10, 2019
@straight-shoota straight-shoota deleted the jm/feature/mime-media-type branch January 10, 2019 07:51
urde-graven pushed a commit to urde-graven/crystal that referenced this pull request Jan 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants