Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

getFile and getStream mirror putFile and putStream #123

Closed
wants to merge 1 commit into from
Closed

getFile and getStream mirror putFile and putStream #123

wants to merge 1 commit into from

Conversation

andrewrk
Copy link
Contributor

see #122.

goals:

  • high level getFile interface just like putFile.
  • getFile emits progress

this is completely untested - looking for preliminary feedback before I figure that stuff out.

@domenic
Copy link
Contributor

domenic commented Nov 21, 2012

Oh, goodness. The first bullet point isn't quite what I had in mind (although it is intriguing).

The problem is Knox uses the "File" suffix in two different ways: one, in getFile/headFile/deleteFile, to mean "file on S3," and two, in putFile, to mean "file from the filesystem" (as opposed to putStream/putBuffer).

I agree this is confusing, and am not sure what the right approach is. I don't think it's to try to move toward the "filesystem" meaning though; that's the minority right now. Maybe putFile should be renamed?? Tricky.

I thought more that getFile could emit progress on the download from S3, while still keeping it's interface.

@andrewrk
Copy link
Contributor Author

Yeah I'm totally willing to compromise on the naming conventions or whatever. But my main issue is with the lack of high level getFile function. Adding a progress event to the existing getFile does not address the root problem IMO.

@domenic
Copy link
Contributor

domenic commented Nov 21, 2012

I mean part of the issue is keeping Knox lean and mean. Node has abstractions for file IO; if we can get you the stream, that should be enough for you to pipe it somewhere. Partially this is hindered by the lack of error detection (see #114), but that's the theory.

@andrewrk
Copy link
Contributor Author

That philosophy is contradicted by having the high level putFile function. Why have one and not the other? What's worse is that the readme advertises getFile as a high level function but it's a lie.

@domenic
Copy link
Contributor

domenic commented Nov 21, 2012

getFile is high-level in relation to get. putFile (which maybe should be named putFromFileName) is in relation to putStream (putFromStream) and putBuffer (putFromBuffer); it's different from get because you actually need something to put, so we can't do a simple layering like we did with get/getFile and head/headFile and del/delFile.

@andrewrk
Copy link
Contributor Author

That is understandable.

So, is a high-level downloadToDisk(remoteFilename, destFilename, callback(error)) function which returns a progress emitter planned, or not?

@domenic
Copy link
Contributor

domenic commented Nov 21, 2012

I don't think so, but as always the @LearnBoost folks are welcome to chime in. If we can solve #114, this should becomes as simple as

knoxClient.getFile(remoteFilename, function (err, res) {
    if (err) { throw err; }
    res.pipe(fs.createWriteStream(destFilename));
});

The key here being that we don't favor fs.WriteStreams over any other type of writable stream.

(And yes, we do favor fs.ReadStreams in putFile, which is a bit unfortunate. It opens up the floodgates to people requesting special methods for all other type of readable streams, e.g. putUrl in #122 for http.IncomingMessage.)

@andrewrk
Copy link
Contributor Author

That code is pretty good. That plus progress events on getFile would be a convenient API.

@andrewrk andrewrk closed this Nov 21, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants