-
Notifications
You must be signed in to change notification settings - Fork 4.1k
Added response headers to onSuccess #160
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
Closed
p-m-j
wants to merge
1
commit into
android-async-http:master
from
p-m-j:d8f606cb1fdd4a97073ab37700e4858daef94e8b
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@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.
To avoid breaking existing client code, it might be better to add this as another overload rather than changing the signature. e.g.
The docstring should also be updated with the new parameter.
The changed formatting in the parameter list is inconsistent with the rest of the library.
Same things with the
JsonHttpResponseHandler
.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.
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)?
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.
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
fromrustybox:response-headers
" instead ofrustybox:d8f606c
.I've made the same mistake in the past; maybe someday github will let you rebase pull requests…
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.
Brilliant, cheers for the help dude I'll get that sorted tomorrow morning (it's pretty late here).
Edit: Closed. reopened as #170