-
Notifications
You must be signed in to change notification settings - Fork 14
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
Add PATCH-processing functionality to Resource. #91
Conversation
e28c343
to
230c502
Compare
else o17 r | ||
|
||
o17 r@Resource{..} = do | ||
trace "o17" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need an updated graph now :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@charleso Sure thing; anyone have an OmniGraffle doc for it? 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious that web machine doesn't appear to support PATCH
right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tmcgilchrist Apparently webmachine is implemented based strictly on RFC 2616, which does not discuss HTTP PATCH
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be nice at some point to tweak for-GET and have the (slightly augmented) airship flow graph.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm… though for-GET is gorgeous, it departs pretty significantly from the Airship graph, AFAICT.
Ugh, sorry for all the whitespace changes. If you really object, I'll disable stylish-haskell (though it's my contention that everyone should have stylish-haskell on everywhere, with editor integration) |
Fixes #90. |
I'm all for consistent styles, whatever they are. Definitely bikeshedding, but (EDIT: our) style guides are more aligned with something like elm. So we don't, for example, align the import values to avoid the big-reshuffle if you import something slightly longer (I don't know how stylish-haskell deals with this, so that may be a moot point). In any case, I'm happy to stick with style-haskell (I don't use it at the moment). You could even go so far to have a pre-commit hook script with instructions for setting up locally to ensure it doesn't get missed. And/or an extra build that fails if something isn't formatted correctly (which might be a little extreme). |
@charleso Cool, I'll leave it in. What I like about stylish-haskell is that it doesn't futz with indentation. It mostly makes small but helpful changes (aligning type declarations, etc.) |
@patrickt re whitespace, as long as it's consistent, I care more about the code being correct. |
👍 |
, processPost :: Webmachine m (PostResponse m) | ||
, processPost :: Webmachine m (PostResponse m) | ||
-- | As with 'processPost', but called on PATCH requests. | ||
, processPatch :: Webmachine m Bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, and this might be part of the RFC, but do you normally care about the content-type for PATCH
in the same way you do with PUT
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@charleso I'd say yes, though I would imagine most people are passing in application/json.
I don't think this fully handles |
Also it looks like you should be calling EDIT Some interesting issues raised here on the ruby web machine client. I don't think the linked diagram is quite right but it does show the other areas that need to change to support PATCH |
Another note: from the RFC,
This is counterintuitive! We presumably need allowMissingPatch :: Webmachine m Bool in the resource type. EDIT: but we will do this later |
783bfe3
to
6565269
Compare
This extends the decision tree with actions to take in case of a PATCH request. If one is encountered, the processPatch member is invoked: if its operation effected no change, it returns a 304 Not Modified; if it did, return a 202 Accepted. Also updates to 0.4.4.0.
2128fc2
to
7157703
Compare
That last commit, connecting o17 to o20, looks right to me. I'd think the PATCH is a rather tricky verb to get right, it'd be good to have an example of how to do it somewhere in the project. |
@tmcgilchrist Re: I can definitely add a PATCH example. |
Cheers, I'm happy with that. 👍 from me |
Add PATCH-processing functionality to Resource.
This extends the decision tree with actions to take in case of a PATCH
request. If one is encountered, the processPatch member is invoked: if
its operation effected no change, it returns a 304 Not Modified; if it
did, return a 204 No Content. This is all invoked directly after testing for
POST.