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

MediaRange immutable #1597

Merged
merged 2 commits into from Dec 20, 2017

Conversation

Projects
None yet
2 participants
@cquiroz
Member

cquiroz commented Dec 16, 2017

This PR removes the use of Registry on MediaRange and converts it up to current standards.

val `text/*` = new MediaRange("text")
val `video/*` = new MediaRange("video")
val standard: Map[String, MediaRange] =

This comment has been minimized.

@rossabaker

rossabaker Dec 17, 2017

Member

I would have thought this includes media types without the second wild card, but I guess it didn't before?

@rossabaker

rossabaker Dec 17, 2017

Member

I would have thought this includes media types without the second wild card, but I guess it didn't before?

This comment has been minimized.

@cquiroz

cquiroz Dec 17, 2017

Member

This is basically replaces the registry with the same set of registered media types as before. Should be fairly mechanical

@cquiroz

cquiroz Dec 17, 2017

Member

This is basically replaces the registry with the same set of registered media types as before. Should be fairly mechanical

This comment has been minimized.

@rossabaker

rossabaker Dec 17, 2017

Member

Hmm. These aren't what I expected them to be. I thought it was keyed by mainType/subType instead of just mainType. I wonder why that is. But you didn't change it.

@rossabaker

rossabaker Dec 17, 2017

Member

Hmm. These aren't what I expected them to be. I thought it was keyed by mainType/subType instead of just mainType. I wonder why that is. But you didn't change it.

This comment has been minimized.

@cquiroz

cquiroz Dec 17, 2017

Member

Well the MediaRange case class only has a mainType unlike MediaType
https://github.com/http4s/http4s/blob/master/core/src/main/scala/org/http4s/MediaType.scala#L97

What surprised me is that the extensions are ignored to set the key

@cquiroz

cquiroz Dec 17, 2017

Member

Well the MediaRange case class only has a mainType unlike MediaType
https://github.com/http4s/http4s/blob/master/core/src/main/scala/org/http4s/MediaType.scala#L97

What surprised me is that the extensions are ignored to set the key

This comment has been minimized.

@rossabaker

rossabaker Dec 17, 2017

Member

A MediaType is a MediaRange, so I'd expect to be able to look up MediaTypes via the MediaRange companion.

I'm not sure what I think about extensions.

@rossabaker

rossabaker Dec 17, 2017

Member

A MediaType is a MediaRange, so I'd expect to be able to look up MediaTypes via the MediaRange companion.

I'm not sure what I think about extensions.

This comment has been minimized.

@cquiroz

cquiroz Dec 19, 2017

Member

Any ideas to move this forward?

@cquiroz

cquiroz Dec 19, 2017

Member

Any ideas to move this forward?

val `text/*` = new MediaRange("text")
val `video/*` = new MediaRange("video")
val standard: Map[String, MediaRange] =

This comment has been minimized.

@rossabaker

rossabaker Dec 17, 2017

Member

Hmm. These aren't what I expected them to be. I thought it was keyed by mainType/subType instead of just mainType. I wonder why that is. But you didn't change it.

@rossabaker

rossabaker Dec 17, 2017

Member

Hmm. These aren't what I expected them to be. I thought it was keyed by mainType/subType instead of just mainType. I wonder why that is. But you didn't change it.

@cquiroz

This comment has been minimized.

Show comment
Hide comment
@cquiroz

cquiroz Dec 20, 2017

Member

This is approved so I could merge it but if you still want some changes related to the key of the standard map please let me know

Member

cquiroz commented Dec 20, 2017

This is approved so I could merge it but if you still want some changes related to the key of the standard map please let me know

@rossabaker

This comment has been minimized.

Show comment
Hide comment
@rossabaker

rossabaker Dec 20, 2017

Member

Let's merge it now because it's forward progress and then discuss more progress.

Am I weird for thinking the standard media ranges should contain media types since media types are media ranges?

Member

rossabaker commented Dec 20, 2017

Let's merge it now because it's forward progress and then discuss more progress.

Am I weird for thinking the standard media ranges should contain media types since media types are media ranges?

@rossabaker rossabaker merged commit 41a7b81 into http4s:master Dec 20, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@cquiroz cquiroz deleted the cquiroz:mediarange-immutable branch Dec 20, 2017

@cquiroz

This comment has been minimized.

Show comment
Hide comment
@cquiroz

cquiroz Dec 20, 2017

Member

I think it is a valid concern

Member

cquiroz commented Dec 20, 2017

I think it is a valid concern

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment