Add support for LWP as an HTTP protocol_class #24
Conversation
@jberger, is this module being developed much? There are PRs from 20 months ago? |
@preaction, I'd prefer you not subdir http.t, which would mean the |
My preference would not to add a dependency on |
Good call. (I'm partly noting the changes I'd like in case I have to take over and finish this PR) |
Also I agree with not renaming http.t I am not sure I understand the purpose of that. |
Yeah, I abandoned my effort to use Alien-Base after I figured out exactly how horrible and evil the library I was trying to download was (I should write a rant about it, but let's start off with "reinvents autotools" and end with "litters GNUMakefile in at least 2 levels of parent directory, which takes precedence in GNU Make over just 'Makefile'"). That said, would Module::Runtime be okay? or just a simple eval? What's wrong with I'm glad I write better commit messages now, because I'm no longer sure why I renamed http.t. From the new name, my guess is going to be I was going to add other ways of determining the package to download. I know for my purposes, I needed a full, direct URL to the tarball. Looks like #26 might be where that got fixed. Let me clean this PR up a bit. I can't do any more work at the moment, but I can at least address these issues quick. |
7ec0fb6
to
a390430
Compare
http.t rename removed and rebased from master. |
I am fine with eval. However, if we are going to be doing a lot of dynamic module then a convenience of some sort is desirable. |
Let's just do: |
Done. |
Is it possible to make the download be fake rather than real? This is an issue raised also on #33, so please don't duplicate effort! Also, I'd like to see a test for this great new functionality. |
I don't know what you mean about faking the download. Do you mean making a mock protocol_class that can be used for testing? I'll write some tests, at least. Since it doesn't look like the tests are fully complete, I may not be able to cover everything. |
I mean something like in the .t file, making a fake LWP::Simple, eg (non-tested example):
So that when you call the relevant AB functionality, it will use that and not the real module. |
I would favor using a Not a strong preference though. |
That's an extremely good idea, do that instead ;-) |
I will probably do both. If the user already has LWP installed, it will test the operation of LWP integration. Either way, we also test a custom connection object (that just happens to be a mock object). I've got tests for connection() written, working on get_file() now. |
Looks like I can't test find_links well without making "file://" act the same as "http://". I should probably make the starting tests a different pull request, then I can write better tests for this pull request on top of that one. |
Speaking for myself, I like this direction. If the PR expands a little beyond the initial concept, I'm fine with that. If others aren't, we can always split some off to a separate PR. For now, carry on with this good work! |
1) Use eval to load the protocol_class requested 2) Add check_http_response to check for HTTP::Response objects This also adds support for file:// URLs, to allow LWP to be tested directly.
Okay, this is what I got now. I can't test HTTP::Tiny via file:// URLs, it does not like it. But LWP works with file://, and indeed the tests revealed a problem in my code which is now fixed. |
I was working on a test module that adds file support to https://github.com/plicease/App-cpangitify/blob/master/inc/Test/HTTPTinyFile.pm It is incomplete, but might be useful here (?), and it would be great to drive development of it. It might also be too Rube Goldberg. |
if using I am all for using subtest I think it makes the output more readable. |
Should LWP be the default, if installed? I don't want LWP to be a prereq, but making it the default if it is installed lets any I think it is still useful to allow an |
I added the Test::More requirement as a separate commit, because subtest was already being used in t/build_flags.t. As for making LWP the default without making it prescribed, HTTP::Tiny purports to now have support for the proxy strategy that $work uses (chansen/p5-http-tiny@30c5500), so in theory this entire PR is unnecessary. I could add a prereq on version 0.048 of HTTP::Tiny. I don't think that the author of the Alien::Foo module should need to make a decision as to which protocol_class to use, but it just worked out that way. In any case, it's not the author of Alien::Foo that could even make that decision, it's the user trying to install the module that would need to make the choice. For me, making an internal Alien::Foo module, I can choose a protocol_class and be certain that (a) it is installed and (b) it solves the problem of navigating the internal network. That might be a valid enough reason to continue this work. My build script is a lot simpler when everything, even external native libraries, are CPAN-style distributions. |
Ah I figured that you were wanting something more sophisticated than basic auth (I reviewed the I think LWP support is still worthwhile, since it has more features than |
No, my initial pull request seems (I can't remember) completely incorrect. The real problem $work has is proxying https through an http proxy via CONNECT requests, which has struggled (there were open bug reports on LWP for years) and finally seems to be pretty well-supported throughout the various modules now (is this another sign of the Perl Revival/Renaissance?) |
I'm favourable towards this too (for what it's worth), so if we can finish this off (once various other merges are done), that would be great! |
I've rebased and updated it so that headers are available in either mode: https://github.com/plicease/Alien-Base/commits/lwp I vote on this version: aye. |
Aye. @zmughal? |
@plicease's branch looks good to me. Merge: Aye |
Add support for LWP as an HTTP protocol_class
Conflicts: lib/Alien/Base/ModuleBuild/Repository/HTTP.pm This merges the changes between Perl5-Alien#24 and Perl5-Alien#55. See Perl5-Alien#56. Merging of [commit](https://github.com/plicease/Alien-Base/commit/a15536c804b9738ae3d45cd9a23ca427855c8927) by @plicease which adds header support for both LWP and HTTP::Tiny. This is needed for getting Content-Disposition information.
Add support for LWP as an HTTP protocol_class
Conflicts: lib/Alien/Base/ModuleBuild/Repository/HTTP.pm This merges the changes between #24 and #55. See #56. Merging of [commit](https://github.com/plicease/Alien-Base/commit/a15536c804b9738ae3d45cd9a23ca427855c8927) by @plicease which adds header support for both LWP and HTTP::Tiny. This is needed for getting Content-Disposition information.
Add support for LWP as an HTTP protocol_class
Conflicts: lib/Alien/Base/ModuleBuild/Repository/HTTP.pm This merges the changes between #24 and #55. See #56. Merging of [commit](https://github.com/plicease/Alien-Base/commit/a15536c804b9738ae3d45cd9a23ca427855c8927) by @plicease which adds header support for both LWP and HTTP::Tiny. This is needed for getting Content-Disposition information.
Add support for LWP as an HTTP protocol_class
Conflicts: lib/Alien/Base/ModuleBuild/Repository/HTTP.pm This merges the changes between #24 and #55. See #56. Merging of [commit](https://github.com/plicease/Alien-Base/commit/a15536c804b9738ae3d45cd9a23ca427855c8927) by @plicease which adds header support for both LWP and HTTP::Tiny. This is needed for getting Content-Disposition information.
Add support for LWP as an HTTP protocol_class
Conflicts: lib/Alien/Base/ModuleBuild/Repository/HTTP.pm This merges the changes between #24 and #55. See #56. Merging of [commit](https://github.com/plicease/Alien-Base/commit/a15536c804b9738ae3d45cd9a23ca427855c8927) by @plicease which adds header support for both LWP and HTTP::Tiny. This is needed for getting Content-Disposition information.
Add support for LWP as an HTTP protocol_class
Conflicts: lib/Alien/Base/ModuleBuild/Repository/HTTP.pm This merges the changes between #24 and #55. See #56. Merging of [commit](https://github.com/plicease/Alien-Base/commit/a15536c804b9738ae3d45cd9a23ca427855c8927) by @plicease which adds header support for both LWP and HTTP::Tiny. This is needed for getting Content-Disposition information.
Alien-Base: Alien-Base: Add support for LWP as an HTTP protocol_class
… plicease/lwp Alien-Base: Alien-Base: Conflicts: Alien-Base: lib/Alien/Base/ModuleBuild/Repository/HTTP.pm Alien-Base: Alien-Base: This merges the changes between #24 and #55. See #56. Alien-Base: Alien-Base: Merging of [commit](https://github.com/plicease/Alien-Base/commit/a15536c804b9738ae3d45cd9a23ca427855c8927) Alien-Base: by @plicease which adds header support for both LWP and HTTP::Tiny. This Alien-Base: is needed for getting Content-Disposition information.
HTTP::Tiny doesn't support authenticating proxies, but LWP::UserAgent does. This patch adds support for other protocol_class possibilities to the HTTP repository, and adds HTTP::Response object support to the API methods.