Skip to content

Conversation

p-m-j
Copy link
Contributor

@p-m-j p-m-j commented Jan 3, 2013

I needed this and spotted a few issue requests for it.

Thanks for the great library :)

Kind regards

Paul

@wuyexiong
Copy link

so cool

@@ -113,7 +114,7 @@ public void onSuccess(String content) {}
* @param statusCode the status code of the response
* @param content the body of the HTTP response from the server
*/
public void onSuccess(int statusCode, String content) {
public void onSuccess(int statusCode,Header[] headers,String content) {
Copy link

Choose a reason for hiding this comment

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

@rustybox, I'm not a maintainer but I have some comments on this commit that I think might make it more likely to be merged in. If you like, I can make the changes and submit a separate pull request, as I'll probably make them anyway for my own use.

  1. To avoid breaking existing client code, it might be better to add this as another overload rather than changing the signature. e.g.

    public void onSuccess(int statusCode, String content) {
        onSuccess(content);
    }
    public void onSuccess(int statusCode, Header[] headers, String content) {
        onSuccess(statusCode, content);
    }
    
  2. The docstring should also be updated with the new parameter.

  3. The changed formatting in the parameter list is inconsistent with the rest of the library.

Same things with the JsonHttpResponseHandler.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey, Thanks for the response, I came to the same conclusions a while back and did those changes in https://github.com/rustybox/android-async-http/commit/78eee0d369f76ca9480c258292c1a959a4da648c.

Is there a way to update the pull request or do I need to do a new one (kind of new to the whole contributing thing)?

Copy link

Choose a reason for hiding this comment

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

Nice. It looks like this pull request is for a specific commit, not a branch, so you might have to create a new one and then close this one (leaving a comment pointing to the new one, e.g. "Reopened as #xx").

I suggest creating a new branch in your repository for this specific pull request (e.g. "response-headers") and then creating the new pull request from that. That way, if you need to make more changes they'll be automatically included. The pull request should look like "into loopj:master from rustybox:response-headers" instead of rustybox:d8f606c.

I've made the same mistake in the past; maybe someday github will let you rebase pull requests…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Brilliant, cheers for the help dude I'll get that sorted tomorrow morning (it's pretty late here).

Edit: Closed. reopened as #170

@p-m-j p-m-j closed this Jan 26, 2013
@p-m-j p-m-j mentioned this pull request Jan 26, 2013
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