Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

0.9 #138

Merged
merged 4 commits into from Jun 6, 2012

Conversation

Projects
None yet
3 participants
Contributor

sopvop commented Jun 5, 2012

Adding PATCH method, and ExtMethod constructor to Methods.

I didn't find if anything else needs to be changed in snap-core.

@gregorycollins gregorycollins and 1 other commented on an outdated diff Jun 5, 2012

src/Snap/Internal/Http/Types.hs
@@ -132,6 +132,7 @@ deleteHeader k = updateHeaders $ H.delete k
-- | Enumerates the HTTP method values (see
-- <http://tools.ietf.org/html/rfc2068.html#section-5.1.1>).
data Method = GET | HEAD | POST | PUT | DELETE | TRACE | OPTIONS | CONNECT
+ | PATCH | ExtMethod ByteString
deriving(Show,Read,Ord,Eq)
@gregorycollins

gregorycollins Jun 5, 2012

Owner

Formatting: please put the pipe at the end of the first line here. Also, would ExtMethod be better as a "CI ByteString"? I don't have any strong feelings one way or the other, just something to think about.

@sopvop

sopvop Jun 5, 2012

Contributor

According to this note method names are case sensitive.
edit: That's the old note, spec 5.1.1 also mentions what methods are case sensitive.

Owner

gregorycollins commented Jun 5, 2012

Could you also please update the test suite to match? There should be tests for ExtMethod at least.

Contributor

sopvop commented Jun 5, 2012

In test I follow formatting as it is there (no spaces in list etc.)

Contributor

afcowie commented Jun 5, 2012

You might want to have a read through snoyberg/http-conduit#24 I'm not pleased with Michael's position there, but it does raise a useful question about whether HEAD is equal to ExtMethod "HEAD". Personally, I see that objection as a bit ridiculous; if you're stupid enough to define one that already exists that's your problem, I should think, but it did come up in other libraries.

Also, why call it ExtMethod? Why not just call the constructor Method Bytestring in the ADT?

AfC

Contributor

sopvop commented Jun 5, 2012

I don't have an opinion about constructor name or equality of HEAD and ExtMethod "HEAD", can make it either way.

@gregorycollins gregorycollins added a commit that referenced this pull request Jun 6, 2012

@gregorycollins gregorycollins Merge pull request #138 from sopvop/0.9
0.9
340f01a

@gregorycollins gregorycollins merged commit 340f01a into snapframework:0.9 Jun 6, 2012

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