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

Jkarni/content types #9

Merged
merged 16 commits into from
Feb 20, 2015
Merged

Jkarni/content types #9

merged 16 commits into from
Feb 20, 2015

Conversation

jkarni
Copy link
Member

@jkarni jkarni commented Jan 27, 2015

Do not merge yet - QQ needs to fixed.

@jkarni jkarni force-pushed the jkarni/content-types branch 2 times, most recently from d702479 to 9785783 Compare February 18, 2015 10:32
@jkarni
Copy link
Member Author

jkarni commented Feb 18, 2015

I'm happy to merge this without QQ so all the downstream packages will start building against master, and then have the QQ as a separate PR. @alpmestan @tvh @christian-marie review?

@tvh
Copy link
Contributor

tvh commented Feb 18, 2015

Where should the instances for MimeRender and MimeUnreder live? I think they should live in a package that is used both by servant-server and servant-client. This could either mean a different package servant-instances, or just moving them in here. I prefer the latter. Related to this: I think there should be only types in here that are actually supported. Having something that sais XML is only confusing if there is no actual implementation backing it.

I also don't like upper bounds where they just guess if it will still work or not. With Stackage we have a powerful tool to catch those broken builds.

Apart from this I like how it looks.

@christian-marie
Copy link
Contributor

I agree to all of these points ^^

I think servant-instances would be not so nice, I'd prefer just sticking them in servant.

As for XML, if it's not got instances it should be added to a servant-xml package with instances. Vice versa probably for anything that isn't JSON. Ideally, I'd like to ship "batteries included".

handleAcceptH _ (AcceptHeader accept) val = M.mapAcceptMedia lkup accept
where pctyps = Proxy :: Proxy ctyps
amrs = amr pctyps val
lkup = zip (map fst amrs) $ map (\(a,b) -> (cs $ show a, b)) amrs
Copy link
Contributor

Choose a reason for hiding this comment

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

This is bending my mind a little, so this may not be 100% correct, but isn't this equivalent to:

fmap (\(a,b) -> (a, (cs $ show a, b)) amrs

Again, I didn't check the types. I may be wrong here.

@thsutton
Copy link
Contributor

I've opened a pull request on servant-docs which updates it in line with the changes here.

https://github.com/haskell-servant/servant-docs/pull/12

@tvh
Copy link
Contributor

tvh commented Feb 19, 2015

There is one for servant-jquery as well.

https://github.com/haskell-servant/servant-jquery/pull/9

@coveralls
Copy link

Coverage Status

Coverage decreased (-10.1%) to 48.48% when pulling 0453cc3 on jkarni/content-types into fb71eb8 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-10.1%) to 48.48% when pulling 0453cc3 on jkarni/content-types into fb71eb8 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.12%) to 57.46% when pulling 2714f34 on jkarni/content-types into fb71eb8 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.12%) to 57.46% when pulling 08528dc on jkarni/content-types into fb71eb8 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+2.61%) to 61.19% when pulling 5470297 on jkarni/content-types into fb71eb8 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+2.61%) to 61.19% when pulling 5470297 on jkarni/content-types into fb71eb8 on master.

@alpmestan
Copy link
Contributor

Great work guys, this looks good to merge! Let the content-type-driven goodness spread =)

@coveralls
Copy link

Coverage Status

Coverage increased (+2.61%) to 61.19% when pulling b02b67b on jkarni/content-types into fb71eb8 on master.

jkarni added a commit that referenced this pull request Feb 20, 2015
@jkarni jkarni merged commit 5f1e8c3 into master Feb 20, 2015
@jkarni
Copy link
Member Author

jkarni commented Feb 20, 2015

Thanks a lot everyone for the reviews and ideas!

@jkarni jkarni deleted the jkarni/content-types branch February 20, 2015 14:38
christian-marie added a commit that referenced this pull request Apr 20, 2015
Add DocIntro and DocNote types to allow extra docs
jkarni added a commit that referenced this pull request Apr 20, 2015
Use 'master' servant and servant-server for travis.
jkarni added a commit that referenced this pull request Apr 20, 2015
Code changes to support Jkarni/content types.
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.

6 participants