Apply environment features to prepared requests

By having trust environment lookup in request() applications using
prepared requests don't get access to this functionality. Having this in
send() is a more logical place as we are dealing with default
communication parameters.

Closes #1436


Hi @jamielennox I haven't looked at this yet because the tests are failing. Please check the failures and fix them. Thank you


@sigmavirus24 fixed, thanks.


There are some nice changes in here. Looking forward to hearing what @sigmavirus24 and @Lukasa have to say :)


This seems in principle to be totally fine. Affects our public facing API in a backward incompatible way though, so we do want to be wary here.


I agree with @Lukasa and want to stress that it's a very subtle way in which it affects the API. Most people who use `Session#send` are not expecting environment based data to affect their request. If they are using prepared requests to avoid this altogether, we're making this basically impossible for them now. We might need to wait for Requests 3.0 to merge this.


What if we compromise. What if instead of moving this into `Session#send` we turn it into a method that returns a dictionary (which is basically `send_kwargs`). We then call that it in `Session#request` and pass it into `Session#send` and users who want this while circumventing `Session#request` can have it.


I realize the backwards compatibility issues you guys have here and if that's what's needed i can make it a function. I had resisted that because there are not a lot of public functions on session and so i hadn't wanted to add this one. 

Having said that there is much simpler way to ignore the environment variables if that was your goal by setting session.trust_env = False, so whilst we may not be able to merge as is i think it's in the spirit of the change.

What about a different way, the code stays much the same as this review, trust_env is a parameter to send(), the default value is False, and the value passed from request() is self.trust_env.


@jamielennox We don't need the `trust_env` keyword argument (in fact I don't think we want it). What we're getting at is that we've moved the location where a specific action occurs, so that previously people who didn't need to worry about the `trust_env` value now do. The fact that it can easily be turned off is not really relevant, we still need to be careful with when we merge the change. I'm happy for us to do it, we should just exercise caution.


@Lukasa Yea, i understand that. I was thinking by doing it this way we didn't change the existing behaviour for anyone and people who wanted to use the environment variables could opt-in. I don't know what the timeframe is but that's preferable to me than waiting for 3.0. 

Do you want me to revert back to the older commit? (wish github had a better way of managing this). 


Yeah, the revert will be better because the new version of the code still changes the API. Previously, `trust_env` was considered in `Session.request`: now it's considered in `Session.send`. Unless we duplicate the code, we can't have it so that there's no change in the API.

Now, that's fine: a change in the API is the whole point of this pull request! =) It just means that we may need to sit on it a little while.


Just for interest (as you're replying quickly), even though in the new version the execution will be exactly the same the fact that the code is executed in a different function constitutes a change in API? What is that designed to protect against - people mocking the adapter.send() function? 


I'm concerned about people subclassing `Session`. =)


I like the idea of making the environment loading a method that would be called manually before running send. This gives people using the send API directly now the ability to get all of the functionality of the standard flow, as well as clean up the code a bit. 

Then we'd keep the behavior identical to what it is now. 


As a side note, breaking this out into another method also means users can override it to do extra things if that suits their desires and we can also test it more easily.


:+1: 


Any update?


@kennethreitz it seems @jamielennox has abandoned this. I'll pull their commit(s) into a branch of my own and update it with our decision tonight.


Sorry everyone, i had let this slip off my list.

This is at least an example of extracting that information to a function, there is a question there as to whether the trust_env check should go inside the apply_environ() function or not, i've gone back and forth. 

I'm not sure if this is what you were looking for - it doesn't feel as 'clean' as the last patches for some reason. 

@sigmavirus24 don't be worried about stepping me through the review process, i'd prefer to get this feature in than worry about who contributed it. If you have something in mind that satisfies the case then merge it and close this out. 


@sigmavirus24 want to own this?


Sure. I'll add this to my TODO list for Madison Ruby today. (Yeah, working on Python at a Ruby conference. =P)


Sweet, closing then :)