Use a ReadCloser to upload assets.

[calavera]

This give us a better abstraction allowing to use multi readers to create things like progress bars.

/cc @jingweno

[owenthereal]

:+1:

[technoweenie]

You're not actually closing it, so perhaps io.Reader would be more appropriate. io.ReadCloser may send a message that upload() will close the handle for you.

This makes more sense since you tend to defer the closing of io streams around the time that you open them.

file, _ := os.open(path)
defer file.Close()
upload(url, file, ctype, clen)

It also means you can pass buffers straight in, without having to use ioutil.NopCloser.

[calavera]

Yeah, the closer is not really necessary, I'm going to change it

[calavera]

Oh, wait, Request.Body requires a io.ReadCloser, see http://golang.org/pkg/net/http/#Request:

// Body is the request's body.
//
// For client requests, a nil body means the request has no
// body, such as a GET request. The HTTP Client's Transport
// is responsible for calling the Close method.
//
// For server requests, the Request Body is always non-nil
// but will return EOF immediately when no body is present.
// The Server will close the request body. The ServeHTTP
// Handler does not need to.
Body io.ReadCloser

[technoweenie]

Sure, but it also satisfies io.Reader. Also, it sounds like Go handles closing it for you for client and server requests.

The HTTP Client's Transport is responsible for calling the Close method.

The Server will close the request body. The ServeHTTP Handler does not need to.

[calavera]

No, it doesn't. You cannot assign an io.Reader to an io.ReadCloser:

octokit/client.go:83: cannot use asset (type io.Reader) as type io.ReadCloser in assignment:
    io.Reader does not implement io.ReadCloser (missing Close method)

octokit/client.go:83:

req.Body = asset

[owenthereal]

From the source, NewRequest first casts to io.ReadCloser, if it's not, converts using ioutil.NopCloser: http://golang.org/src/pkg/net/http/request.go?s=13983:14055#L434.

[technoweenie]

@calavera: You can assign an io.ReadCloser to an io.Reader though. This change won't prevent you from writing http request bodies.

@jingweno: Yes, io.ReadCloser makes sense for http request bodies. I'm just saying it's not necessary for this upload() function.

[technoweenie]

Oh, you're setting it directly to a request body. I seeeee. Carry on, lol.

[technoweenie]

I realize where my confusion came from. We had a similar PR on an API like this (converting an io.ReadCloser to an io.Reader). We can get away with just io.Reader because my write function is wrapping the io.Reader with a custom io.ReadCloser that calculates the sha1 of the file. That is what we set to Request.Body.

[owenthereal]

@technoweenie I'm gonna merge this one now and let's create another PR if we need to support accepting io.Reader as params.