use requests library?

[imnotjames]

Instead of using open, and the util.http module for handling HTTP requests, would there be interest in converting this over to using the requests library? It means a more generalized library with more control over the workflow.

If so, we could utilize requests-mock to create mocks more easily for tests for the plugins.

A branch using requests in the util.http can be found here https://github.com/imnotjames/skybot/tree/feature/use-requests-for-http but I think there's a benefit in handling everything with requests directly.

Thoughts, concerns, etc?

[Red-M]

Yes, with an opt-in style of implementation.

…

On Thu., 9 Aug. 2018, 12:24 pm James Ward, ***@***.***> wrote: Instead of using open, and the util.http module for handling HTTP requests, would there be interest in converting this over to using the requests <https://github.com/requests/requests/> library? It means a more generalized library with more control over the workflow. If so, we could utilize requests-mock <https://github.com/jamielennox/requests-mock> to create mocks more easily for tests for the plugins. I've already put some work into making this happen in the ppp-helpdesk fork, but it means installing more dependencies. Thoughts, concerns, etc? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#181>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABZoEcci_revFxj47DmieTekbCyuVb29ks5uO51CgaJpZM4V08A3> .

[imnotjames]

What do you mean by an opt-in style of implementation? Leaving everything in the util.http alone and migrating all of the existing plugins over?

[Red-M]

If requests is installed then use it, otherwise default to httplib

…

On Thu., 9 Aug. 2018, 12:40 pm James Ward, ***@***.***> wrote: What do you mean by an opt-in style of implementation? Leaving everything in the util.http alone and migrating all of the existing plugins over? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#181 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABZoEdqgDCwM8uF4FPiekw19BrM1JbRdks5uO6EZgaJpZM4V08A3> .

[imnotjames]

Hmm.. well, I don't really see anyone would start using requests at that point - since it just means keeping up two paths rather than one. Instead of simplifying development, it complicates it.

If that's the only path forward, I don't think it makes sense to go with requests in that case.

[Red-M]

If I remember correctly py2 doesn't have requests installed by default. The opt-in is during the phase of py2 still being supported once py2 sunsets, then make requests the default and remove the httplib dependency.

…

On Thu., 9 Aug. 2018, 2:27 pm James Ward, ***@***.***> wrote: Hmm.. well, I don't really see anyone would start using requests at that point - since it just means keeping up two paths rather than one. Instead of simplifying development, it complicates it. If that's the only path forward, I don't think it makes sense to go with requests in that case. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#181 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABZoEaG8VjPgLtnXk-IJSrJ1M7ly7Q7uks5uO7pDgaJpZM4V08A3> .

[imnotjames]

Python 3 doesn't have it installed by default either, so I don't see much of a difference, myself. Is it too not change the dependencies until another large breaking change is introduced? My reasoning for suggesting this, is it's a wrapper library to simplify making http requests, which is what the utilities class is. It's just more well known.

…

On Thu, Aug 9, 2018, 4:00 AM Red_M ***@***.***> wrote: If I remember correctly py2 doesn't have requests installed by default. The opt-in is during the phase of py2 still being supported once py2 sunsets, then make requests the default and remove the httplib dependency. On Thu., 9 Aug. 2018, 2:27 pm James Ward, ***@***.***> wrote: > Hmm.. well, I don't really see anyone would start using requests at that > point - since it just means keeping up two paths rather than one. Instead > of simplifying development, it complicates it. > > If that's the only path forward, I don't think it makes sense to go with > requests in that case. > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > <#181 (comment)>, or mute > the thread > < https://github.com/notifications/unsubscribe-auth/ABZoEaG8VjPgLtnXk-IJSrJ1M7ly7Q7uks5uO7pDgaJpZM4V08A3 > > . > — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#181 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABes6SqyiIn5dlTy6iemPtdQ2Lmyf3k7ks5uO-wVgaJpZM4V08A3> .

[Red-M]

Look at the code for util.http ( https://github.com/rmmh/skybot/blob/master/plugins/util/http.py) and you'll realise that you're going to need to replace urllib(2) with requests (if requests is installed) because otherwise you'll have to rewrite every interaction with util.http for each plugin. Also that every bot downstream or that was compatible with skybot's plugins also now has to change, not that this is a primary reason but its also something to consider. On Fri, Aug 10, 2018 at 12:14 AM, James Ward <notifications@github.com> wrote:

…

Python 3 doesn't have it installed by default either, so I don't see much of a difference, myself. Is it too not change the dependencies until another large breaking change is introduced? My reasoning for suggesting this, is it's a wrapper library to simplify making http requests, which is what the utilities class is. It's just more well known. On Thu, Aug 9, 2018, 4:00 AM Red_M ***@***.***> wrote: > If I remember correctly py2 doesn't have requests installed by default. > > The opt-in is during the phase of py2 still being supported once py2 > sunsets, then make requests the default and remove the httplib dependency. > > On Thu., 9 Aug. 2018, 2:27 pm James Ward, ***@***.***> > wrote: > > > Hmm.. well, I don't really see anyone would start using requests at that > > point - since it just means keeping up two paths rather than one. Instead > > of simplifying development, it complicates it. > > > > If that's the only path forward, I don't think it makes sense to go with > > requests in that case. > > > > — > > You are receiving this because you commented. > > Reply to this email directly, view it on GitHub > > <#181 (comment)>, or > mute > > the thread > > < > https://github.com/notifications/unsubscribe-auth/ABZoEaG8VjPgLtnXk- IJSrJ1M7ly7Q7uks5uO7pDgaJpZM4V08A3 > > > > . > > > > — > You are receiving this because you authored the thread. > Reply to this email directly, view it on GitHub > <#181 (comment)>, or mute > the thread > <https://github.com/notifications/unsubscribe-auth/ ABes6SqyiIn5dlTy6iemPtdQ2Lmyf3k7ks5uO-wVgaJpZM4V08A3> > . > — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#181 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABZoERE4MG5qKy4Z2arC8LCKTRtoJtB4ks5uPEPFgaJpZM4V08A3> .

[imnotjames]

Right - I did that in util.http while updating the requirements.txt, and in another fork I've gone ahead and updated some if the built in plugins, which spurred my opening of this issue.

I think that as an added benefit, other bots would be less tied to skybot-specific code if they wanted to use the plugins system. They'd just need to install requests, which is a bit more standard of a library than util.http. I don't think that's more of a problem than knowing you need to manually copy over util.http and keep that up to date with the skybot version.

I'll research how we can mock the urllib HTTP. Maybe that would be a better place to start so we can get tests in place to verify we aren't causing breakage within the plugins of we were to make a change like this.