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

feat(client): add patch method to Client builder interface #659

Merged
merged 1 commit into from
Oct 2, 2015

Conversation

softprops
Copy link
Contributor

I'm building a client interface on top of hyper for an rest API that makes use of the HTTP PATCH method. It felt a little awkward that there were methods for get post and delete but not patch so I wanted to make it less awkward for others.

@seanmonstar
Copy link
Member

I don't mean to be argumentative, I really do appreciate the PRs.

I just want to talk through this first. It seems like a tiny amount of code, who cares, but it slowly expands the surface area of the API (and the documentation). So, is it awkward to have to use client.request(Method::Patch, ...)? I must say, in my years as web developer, I've never actually even needed to reach for the PATCH verb. Also, when I look around at other Client APIs, the convenient methods I see are the ones already here in hyper. I don't recall seeing one for PATCH or others.

I could just be being an unintentional jerk, and should just merge this.

@softprops
Copy link
Contributor Author

I totally get the thing about expanding surface area and documentation.

From a library user's perspective though, I'd say it's not awkward because client.request(Method::Patch, ...) is hard to type. It's awkward because because it's inconsistent with the rest of the hyper's provided client api.

If the other HTTP method helpers were added as "convenience", at some point, I'd assume it was decided that client.request(Method::Get, ...) was an inconvenient interface. The lack of a the same convenience when trying to accomplish the same task led to a sense of inconsistency for me. That's what felt awkward.

In particular, I'm interface with Github's api which uses PATCH extensively, if not exclusively, for partial updates to resources.

@softprops
Copy link
Contributor Author

I'm also not trying to be argumentative. I'm just trying to provide a consistent experience for future hyper users :)

@reem
Copy link
Contributor

reem commented Oct 2, 2015

My 2c: I would not mind adding a convenience for patch, it seems mostly harmless to me; I am not very concerned about the maintenance burden of such a method.

@seanmonstar
Copy link
Member

I wasn't worried about maintenance, but rather "when a user goes to the Client doc page, does extra convenience methods help, or provide noise." As in, do the convenience methods pull their weight. I'll merge this, but just wanted to bring it up since I imagine a similar PR adding a method for the CHECKOUT or other obscure method would be denied.

seanmonstar added a commit that referenced this pull request Oct 2, 2015
feat(client): add patch method to Client builder interface
@seanmonstar seanmonstar merged commit 388ddf6 into hyperium:master Oct 2, 2015
@reem
Copy link
Contributor

reem commented Oct 2, 2015

It's definitely a legitimate concern generally speaking, but I think it is worth it in this case.

@softprops
Copy link
Contributor Author

when a user goes to the Client doc page, does extra convenience methods help, or provide noise

If its of any value it was the lack of a doc for this method that forced me to read through the source code to figure out the method didn't actually exist. As a user, it just felt like it should exist.

@seanmonstar
Copy link
Member

If its of any value it was the lack of a doc for this method that forced me to read through the source code to figure out the method didn't actually exist. As a user, it just felt like it should exist.

Oh? Interesting. My initial reaction when using any method thats not one of the big 4 (get, post, put, delete) is to look for the method that lets me specify the exact method I want...

@softprops
Copy link
Contributor Author

It's that kind of inconsistency that makes it's absence feel awkward.

@softprops
Copy link
Contributor Author

thanks @seanmonstar !

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.

3 participants