Add Get() source for retriving web resources #3
Conversation
bitfield left a comment
bitfield left a comment There was a problem hiding this comment.
Fantastic PR, thank you! It's much appreciated. I'd love to hear what your use case is for this feature—it's helpful when considering how to design the feature, and it may also suggest useful example programs that we can include.
sources_test.go Outdated
| } | ||
| | ||
| func TestGet(t *testing.T) { | ||
| defer gock.Off() |
There was a problem hiding this comment.
Would you mind very much using plain old httptest for this instead? I'm rather pleased with the fact that script is currently a zero-import library.
There was a problem hiding this comment.
There was a problem hiding this comment.
So you would rather do a full integration test here instead of a test with mocks (avoiding any 3rd-party dependencies)?
There was a problem hiding this comment.
No, an integration test would actually call out to the network. I'm suggesting using a local HTTP server, as you do here; just not using the gock library.
sources.go Outdated
| return Echo(s.String()) | ||
| } | ||
| | ||
| // Get creates a pipe containing the output of retrieving the web resource |
There was a problem hiding this comment.
Can you expand on this a bit (and also update the README)? See the documentation for existing methods for the sort of thing I'm looking for, including error handling, for example.
| // given by the supplied url. | ||
| func Get(url string) *Pipe { | ||
| p := NewPipe() | ||
| res, err := http.Get(url) |
There was a problem hiding this comment.
One thing that concerns me about this is the lack of a timeout in the default HTTP client, for example: https://medium.com/@nate510/don-t-use-go-s-default-http-client-4804cb19f779
The way I've tackled this in other programs is to have the default behaviour be to use the default client, but allow the user to override this by creating their own client and injecting it somehow. What do you think we should do here?
| if err != nil { | ||
| return p.WithError(err) | ||
| } | ||
| return p.WithReader(res.Body) |
There was a problem hiding this comment.
An interesting question here is whether it makes sense (or is possible) to expose the HTTP status code to the user somehow. Usually if you're calling something like Get(), you care if the result is 404, for example. Any thoughts?
There was a problem hiding this comment.
I don't see how that's possible right now with the current design of Pipe. I don't just want to go around adding random methods to it that have no value for other types of sources, filters and sinks.
One sensible approach would be to more closely mirror the behaviour of curl and just return all of the output, headers, status line, body, the lot.
There was a problem hiding this comment.
Yes, that's an option. I think it's likely to be more useful if Get() simply returns the document, without headers. For example, if you're downloading an install script, you could Get() it, save it to a file, then Exec() it. You don't want to have to strip out headers and status info.
On the other hand, if the download fails altogether (404, for example), there's no point carrying on. We need some way to detect this. How about setting the error status on the pipe if the HTTP status indicates a problem?
sources_test.go Outdated
| func TestGet(t *testing.T) { | ||
| defer gock.Off() | ||
| | ||
| gock.New("http://localhost:8000"). |
There was a problem hiding this comment.
Can we add a few more test cases here?
There was a problem hiding this comment.
Such as? This test fully tests the codepath thus far; I see no value in testing anything else.
There was a problem hiding this comment.
There are at least a few things I can think of; redirects and 404 errors, for example. Does Get() follow 3xx redirects?
To be brutally honest; I was just curious about your project, read the README and implemented what you documented as "suggested new features". I have no use-case at present. Also the above you've almost copied/pasted word-for-word in every PR I've seen so far :) You might want to think about not doing that so much :D It's a little off putting... |
Well, it's an excellent principle of library design (which I'm trying to follow in this library) not to add a feature just because it's cool, or you think it might be useful, but only when you have an actual requirement for it. The point of this (apart from helping to avoid cluttering up the API with unnecessary things) is that a real-world use case gives you information about how best to design the feature. It's not till you try and use the function yourself that you realise what the API should have been! |
Well, given that every PR needs to be driven by a use case, what would be a better way to communicate this information, or ask this question? |
| Hey @prologic—are you still interested in working on this? If not, that's fine, but would you mind if I picked up your PR and finished off the changes requested here? |
| By all means :) Go right ahead! James Mills / prologic E: prologic@shortcircuit.net.au W: prologic.shortcircuit.net.au …On Wed, Jul 3, 2019 at 2:12 AM John Arundel ***@***.***> wrote: Hey @prologic <https://github.com/prologic>—are you still interested in working on this? If not, that's fine, but would you mind if I picked up your PR and finished off the changes requested here? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3?email_source=notifications&email_token=AAJ276VAPKGUPETWCKXJHOTP5N45RA5CNFSM4HWS2YJKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZBZEOI#issuecomment-507744825>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJ276QZR42CQ5HOZ63BBV3P5N45RANCNFSM4HWS2YJA> . |
| Closing this one for now, as this feature would benefit from a bit more thought and discussion; when the occasion for that comes up, we can refer to the excellent start made by this PR. Thank you @prologic. |
2 participants
No description provided.