Skip to content

Add MIME::MediaType#7077

Merged
sdogruyol merged 2 commits into
crystal-lang:masterfrom
straight-shoota:jm/feature/mime-media-type
Jan 10, 2019
Merged

Add MIME::MediaType#7077
sdogruyol merged 2 commits into
crystal-lang:masterfrom
straight-shoota:jm/feature/mime-media-type

Conversation

@straight-shoota
Copy link
Copy Markdown
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

Comment thread spec/std/mime/media_type_spec.cr Outdated
Comment thread src/mime/media_type.cr Outdated
Comment thread src/mime/media_type.cr Outdated
Comment thread src/mime/media_type.cr Outdated
Comment thread src/mime/media_type.cr Outdated
@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
Copy Markdown
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

Comment thread src/mime/media_type.cr Outdated
Comment thread src/mime/media_type.cr Outdated
Comment thread src/mime/media_type.cr Outdated
Comment thread src/mime/media_type.cr Outdated
Comment thread src/mime/media_type.cr Outdated
# A MediaType describes a MIME content type with optional parameters.
struct MediaType
property media_type : String
property params : Hash(String, String)
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Comment thread src/mime/media_type.cr Outdated
Comment thread src/mime/media_type.cr Outdated
@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
Copy Markdown
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
Copy Markdown
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.

Comment thread spec/std/mime/media_type_spec.cr
Comment thread src/mime/media_type.cr Outdated
Comment thread src/mime/media_type.cr Outdated
Comment thread src/mime/media_type.cr Outdated
Comment thread src/mime/media_type.cr Outdated
Comment thread src/mime/media_type.cr Outdated
Comment thread src/mime/media_type.cr Outdated
Comment thread src/mime/media_type.cr Outdated
Copy link
Copy Markdown
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 👍

Comment thread src/mime/media_type.cr
@RX14 RX14 added this to the 0.27.1 milestone Dec 20, 2018
@RX14
Copy link
Copy Markdown
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.

5 participants