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

Emphasize client #109

Closed
wants to merge 6 commits into from
Closed

Conversation

cwaldbieser
Copy link
Contributor

Based on a conversation with Glyph in PR #76, I reverted the changes from that PR and instead updated the examples and documentation to emphasize that client code may interact with treq.client.HTTPClient directly. In fact this should be the way to customize the behavior of the client. The various treq request functions are just shortcuts.

Glyph pointed out that the various treq request functions (`treq.get()`,
`treq.post()`, etc.) don't need to be modified.  Instead, the documentation
should suggest that you use a `treq.client.HTTPClient` instance directly
if you need to customize its behavior.

This reverts commit 972b84f.
@codecov-io
Copy link

Current coverage is 96.30%

Merging #109 into master will decrease coverage by -0.01% as of 8aba93c

@@            master   #109   diff @@
=====================================
  Files           19     19       
  Stmts         1601   1595     -6
  Branches       121    120     -1
  Methods          0      0       
=====================================
- Hit           1542   1536     -6
  Partial         19     19       
  Missed          40     40       

Review entire Coverage Diff as of 8aba93c

Powered by Codecov. Updated on successful CI builds.

@glyph
Copy link
Member

glyph commented Jul 30, 2015

@cwaldbieser thanks for this update! I just merged another PR that conflicts with this, unfortunately; do you want to remove the agent parameter first? @hynek - what do you think?

@hynek
Copy link
Member

hynek commented Jul 30, 2015

I’m confused why it removes the custom agent test?

I’m also not a fan of replacing treq by a http_client in all examples TBH. It should be documented but this is excessive.

@cwaldbieser
Copy link
Contributor Author

The custom_agent test was reverted along as part of the change that added
passing agent as a parameter to the shortcut functions. The test is no
longer valid.

I could put all the examples back except the one specific to docs on how
you customize the underlying agent. Would that be better?

Thanks,
Carl

On Thu, Jul 30, 2015 at 2:51 AM, Hynek Schlawack notifications@github.com
wrote:

I’m confused why it removes the custom agent test?

I’m also not a fan of replacing treq by a http_client in all examples TBH.
It should be documented but this is excessive.


Reply to this email directly or view it on GitHub
#109 (comment).

@cwaldbieser
Copy link
Contributor Author

I merged the recent change to master back into this branch.
I removed all traces of passing the custom agent as a parameter to the treq.(get|post|put|delete|...) methods. Instead, I updated the docs to explain how you should just use treq.client.HTTPClient directly, as this was not particularly clear until @glyph pointed this out.
I also reverted the other examples back to using treq.get() etc., as @hynek is right-- it did feel like overkill to just bind an additional name in all the examples that don't use a custom agent.

@glyph
Copy link
Member

glyph commented Oct 10, 2015

Oh no, more conflicts :-(. This is actually looking pretty good though, can you resolve the conflicts again and perhaps we can land it?

@cwaldbieser
Copy link
Contributor Author

I corrected the conflict and pulled out the code that would allow you to pass a custom agent to the short cut functions in api.py. The docs explain that the way to customize the client behavior is to instantiate an HTTPClient directly and not use the short cuts.

@glyph
Copy link
Member

glyph commented Jul 12, 2016

@cwaldbieser - I was ready to merge this after resolving conflicts... but then I saw you broke compatibility by removing the agent= functionality. That is at the very least a separate change :-. Sorry about the ridiculous review latency on Treq, we have not had a good process to encourage people to look over pull requests.

@glyph
Copy link
Member

glyph commented Jul 12, 2016

I feel bad about asking you to do something about this only every 6 months and then not merging it, so I've taken over the work in #129 .

@glyph
Copy link
Member

glyph commented Jul 12, 2016

Closing so as not to have a duplicate, but your work is appreciated :).

@glyph glyph closed this Jul 12, 2016
pawelmhm pushed a commit to pawelmhm/treq that referenced this pull request Nov 4, 2016
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.

None yet

4 participants