Skip to content

Add NoContentVerb#1219

Closed
fierce-katie wants to merge 6 commits intohaskell-servant:masterfrom
fierce-katie:no-content-verb-#1028
Closed

Add NoContentVerb#1219
fierce-katie wants to merge 6 commits intohaskell-servant:masterfrom
fierce-katie:no-content-verb-#1028

Conversation

@fierce-katie
Copy link
Contributor

@fierce-katie fierce-katie commented Sep 7, 2019

Closes #1028.

The NoContentVerb endpoints always return status code 204 and empty body and it is reflected in the docs.

@fierce-katie
Copy link
Contributor Author

fierce-katie commented Sep 8, 2019

The only failing build on Travis CI is the one for ghc-8.0.2. It fails with

internal error in InstallPlan.completed
CallStack (from HasCallStack):
  error, called at ./Distribution/Client/InstallPlan.hs:262:25 in main:Distribution.Client.InstallPlan

I have not changed the CI configuration.

Probably related to haskell/cabal#6060

phadej
phadej reviewed Sep 8, 2019
View reviewed changes
Copy link
Contributor

@phadej phadej left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I don't have an opinion on this

Probably this is a right thing to do now.

#841 should make this unnecessary, but it's a holy grail which would fix everything, yet not founded still :)

I think that we should remove NoContent type if we go with this. ping @alpmestan

EDIT: above doesn't need to be done in this PR; also I'll figure what's wrong with CI myself

servant-server/src/Servant/Server/Internal.hs Outdated
where method = reflectMethod (Proxy :: Proxy method)
status = toEnum . fromInteger $ natVal (Proxy :: Proxy status)

instance {-# OVERLAPPABLE #-}
Copy link
Contributor

@phadej phadej Sep 8, 2019

Choose a reason for hiding this comment

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

OVERLAPPABLE/OVERLAPPIG (here and elsewhere) are not required, are they?

@alpmestan
Copy link
Contributor

alpmestan commented Sep 8, 2019

I like this patch, because users don't have to specify an arbitrary content type anymore.

@phadej phadej mentioned this pull request Sep 29, 2019
Merged
@phadej phadej added this to the 0.17 milestone Sep 29, 2019
@phadej
Copy link
Contributor

phadej commented Sep 29, 2019

Thanks. Merged rebased version in #1228 (CI was green there).

@phadej phadej closed this Sep 29, 2019
@phadej phadej mentioned this pull request Sep 30, 2019
Merged
@phadej phadej mentioned this pull request Dec 14, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@phadej phadej phadej left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.17

Development

Successfully merging this pull request may close these issues.

servant-docs: Incorrect docs generated for NoContent responses

3 participants

@fierce-katie @alpmestan @phadej