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

Provide Method to update the URL in a RequestBuilderWrapper and maintain modifications to the BoundRequestBuilder #57

Merged
merged 1 commit into from
Jun 3, 2017

Conversation

rabeyta
Copy link
Contributor

@rabeyta rabeyta commented May 30, 2017

See #56 for the background.

I have two commits here per my initial analysis and suggestions in #56 . Based on feedback I can keep them both, remove one, or change the PR to a more optimal solution.

edit - removed the one commit i didn't like after discussion in the issue and updated the doc for the set url method

@codecov-io
Copy link

codecov-io commented May 30, 2017

Codecov Report

Merging #57 into master will increase coverage by 0.17%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #57      +/-   ##
============================================
+ Coverage      90.2%   90.38%   +0.17%     
- Complexity     1947     1958      +11     
============================================
  Files           141      141              
  Lines          5790     5804      +14     
  Branches        765      765              
============================================
+ Hits           5223     5246      +23     
+ Misses          387      379       -8     
+ Partials        180      179       -1
Impacted Files Coverage Δ Complexity Δ
...e/client/asynchttp/ning/RequestBuilderWrapper.java 100% <100%> (ø) 11 <8> (+8) ⬆️
...ient/asynchttp/netty/StreamingAsyncHttpClient.java 73.42% <0%> (+0.46%) 38% <0%> (+1%) ⬆️
...oste/server/handler/IdleChannelTimeoutHandler.java 92.3% <0%> (+53.84%) 4% <0%> (+2%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e57e93...cdfcfc6. Read the comment docs.

@nicmunroe
Copy link
Member

Based on our discussion in issue #56, go ahead and add the getters/setters to RequestBuilderWrapper (option 1) and get rid of the option 2 code. And while you're in there we might as well make everything mutable (HTTP method, circuit breaker stuff). If HTTP method turns out to really truly not be mutable then that's fine, just ignore it.

… that sets the url internally and also on the wrapped BoundRequestBuilder
@rabeyta
Copy link
Contributor Author

rabeyta commented Jun 2, 2017

@nicmunroe Sounds good. i've updated the PR with those changes. Please take a look and let me know if you can think of any other changes needed.

@nicmunroe
Copy link
Member

Awesome. Thank you as always!

@nicmunroe nicmunroe merged commit a9caa1e into Nike-Inc:master Jun 3, 2017
@rabeyta rabeyta deleted the issue56 branch June 5, 2017 15:34
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