POC: Vendor RequestEncodingMixin from requests

[Bobronium]

See discussion in #174

There's code from requests as well as utllib3. The sole purpose of this PR is to demonstrate minimal amount of work required to have testing working without requests and urllib3 installed.

[peterschutt]

Thanks @Bobronium - I'll have time to have a closer look later tonight my time (+10), here's what I'd be changing on first pass.

• change starlite/testing.py to a package with starlite/testing/{__init__.py,testing.py,_utils/_request_encoding_mixin.py}
• ensure the functional api of starlite.testing is identical to the api of starlite/testing.py so there's no breakages.
• minimize as much as possible and ensure that we only have as much behavior vendored for what we need.
• checkout what the licensing requirements are for those upstream libs and attribute/include licences as necessary. I've seen this stuff included in module docstrings.
• put a note in the docs along side the testing client that says not to use the client in production, it's included as a convenience for testing only.
• remove this bit from the docs "which in turn is built using the [requests](https://docs.python-requests.org/en/latest/) library."
• ensure that we depend on starlette[full] (should that be only in testing extras?) if we need it for their own code that we depend on.

That's pretty much the solution I'd be happy to debate on. I'll admit the LOC is larger than I anticipated, when I skimmed the source I read urllib3 as stdlib's urllib so that's on me, but I'm still here. Here's why.

Having the module scope __getattr__ logic in our library index is a big flag to anyone who looks that says, "hey, come check out this weird thing that we are doing so that we can rely on a single class of the entire requests library for our testing client." The logic in starlite/__init__.py is like the front-page of the whole library, and we should prefer to keep it concise and tidy if possible. Further, if it's not deprecated it's there forever. This library is still young, and evolving. Trying to limit the amount of up-front technical crud we add, particularly closer to the top of the front-line library namespace should be a feature of our work.

Let me qualify that this is all "IMO". I respect that your opinions may differ, but I think it is important that if I don't agree, I'll express why. I'm happy for my mind to be changed.

I think your original issue that led to this work is totally valid and I'm 100% on board. I'd feel better about having a http client lib included in our dependencies if it could do what we needed, and was async, but we are specifically talking about regular old requests here.

[Bobronium]

Just to be clear, I'm not planning to continue working on this PR, so please feel free to push changes to vendor-request-encoding-mixin if you're a maintainer, or open a separate PR.

[peterschutt]

For sure, but do you have any opinion on the discussion points I've raised?

[Bobronium]

If we're going with vendoring approach, I fully agree with points in bullet list.

As a user, I preffer starlite to vendor requests, so I could deal with less dependencies :)

As for __getattr__ discussion, It is kind of sad that you're concerned that it's going to be percieved as something weird. It definitely not a popular thing, but I think only because not many people knows about it in the first place. It solves real problems like library import time, module level deprecations, etc., and spreading awarness about it existance is rather a good thing, IMO.

To be clear, I'm not implying that starlite should have module __getattr__ to spread awareness nor that it should have it at all. Just sharing my thoughts on the topic, so somebody who reads it won't think "I don't want to use this quirky thing, people gonna look at me funny", when there's gonna be a real use case for it \o/

[peterschutt]

As for getattr discussion, It is kind of sad that you're concerned that it's going to be percieved as something weird.

Sure that's a valid point, what if I rephrased that as "a tool that is maybe overkill for this application"? I'm happy to use it in the right contexts, I'm just not sure that a workaround for a small testing dependency is a good use of it, and worry that it is promoting the quirkyness of needing requests for this one small thing in the first place.

How would you feel if urllib3 was a dependency? That would be a good balance between minimising the vendored code and keeping the dependencies of the library simple. So we depend on urllib3 and vendor the requests class, or does that bring us right back to the start again?

[Bobronium]

Sure that's a valid point, what if I rephrased that as "a tool that is maybe overkill for this application"? I'm happy to use it in the right contexts, I'm just not sure that a workaround for a small testing dependency is a good use of it, and worry that it is promoting the quirkyness of needing requests for this one small thing in the first place.

I don't see how using __getattr__ is an overkill and vendoring parts of requests and urllib3 is not (purely from required effort perspective). For me it seems like vendoring will require more effort.

Maybe we're looking at this from different perspectives?

I'm looking at this from where we are now and what we can do next, and I'm guessing you're looking at this from future perspective, e.g. when one could see only the end result without seeng a path to this result, as if the state where project ended up was desired from the beginning.

[Goldziher]

Hmm. So, this is quite a lot of third party code. I don't want this in our code base, because it's "dirty".

[peterschutt]

@Goldziher i agree that I don't want to pull in all that urllib3 code either.

@Bobronium my question earlier was:

How would you feel if urllib3 was a dependency?

Requests dependencies:

requires = [
    "charset_normalizer~=2.0.0",
    "idna>=2.5,<4",
    "urllib3>=1.21.1,<1.27",
    "certifi>=2017.4.17",
]

Urllib3 dependencies:

requires=[]

But this still won't solve the issue of us providing requests for starlette's test utilities because we don't require their full extras, if that is a problem.

when one could see only the end result without seeng a path to this result

I suppose that sums my position up.

I've stated my position pretty clearly, I don't like having the getattr at the package namespace level just to support this quirk that we need requests for a testing utility.

You have the option of going ahead with the getattr solution.

We still have the option of doing nothing.

We also have the option of making a breaking change and minimising the stuff we expose in the package namespace. We could prob use the exercise to clean up some of the class interfaces in terms of what are public and private attributes as well.