Skip to content
This repository has been archived by the owner on Feb 1, 2019. It is now read-only.

CSV Exporting #118

Merged
merged 20 commits into from
Apr 9, 2015
Merged

CSV Exporting #118

merged 20 commits into from
Apr 9, 2015

Conversation

stevejameskent
Copy link
Contributor

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 98.61% when pulling 9ac0096 on stevejameskent:csv into e7391d1 on cfpb:milestone9.

* @param {object} writeStream Handle to a {@link https://nodejs.org/api/stream.html#stream_class_stream_writable_1|stream.Writable} instance to output to
* @see {@link CSVProcessor|CSVProcessor} for more info
*/
HMDAEngine.prototype.exportAll = function(year, errorType, writeStream) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know we called it "export all" in our discussions, but since this function is limited to the errorType, should we change the name to exportType?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@poorgeek
Copy link
Contributor

poorgeek commented Apr 7, 2015

So my main concern with this is that it the CSVProcessor requires a write stream to be passed into it. I've already looked and found that there are ways to create streams in the browser, but since the file needs to be complete before we can even download it, would it make more sense to just have the engine return a string?

@stevejameskent
Copy link
Contributor Author

@poorgeek I had it use a write stream because that's the easiest way to work with the csv module as it's all modeled after the node stream api. Since every component in the chain is a stream they implicitly handle the details of passing data between them so we don't have to. It also means that once the end points close the stream every component in the chain is flushed and closed as well so we don't have to worry about any open handles or memory leaks.
When running in node the write stream can be a handle to the actual output file which is very convenient. I was hoping there would be a way in the UI to prompt the user to select a location and filename and then pass a handle to a write stream for that file to the endpoints. If we can't do that and need to buffer the output instead we can still handle that; we'll need to build a simple write stream object that uses a string as it's storage (see the unit tests for an example). We'll also have to change the endpoints to include a callback as well so that the engine can push the result string back to the UI.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 98.63% when pulling 5c7b49b on stevejameskent:csv into e7391d1 on cfpb:milestone9.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 98.62% when pulling aa090e8 on stevejameskent:csv into e7391d1 on cfpb:milestone9.

@LinuxBozo
Copy link
Contributor

@stevejameskent After thinking through this a bit, I think the API should consist of 4 functions..

  1. returns a readable stream for individual edit (params: errorType, errorId)
  2. returns a readable stream for a type (param: errorType)
  3. returns a promise for a string for individual edit (params: errorType, errorId)
  4. returns a promise for a string for a type (param: errorType)

Shouldn't need year param, as you can use .getRuleYear()

In the case of 1 and 2, you can take that stream and pipe it elsewhere (like on a server, to an http stream for a response) and in the case of 3 and 4, you can .then() do something with the full csv string (like in the UI)

@poorgeek
Copy link
Contributor

poorgeek commented Apr 8, 2015

👍

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 98.64% when pulling 96933c4 on stevejameskent:csv into e7391d1 on cfpb:milestone9.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 98.64% when pulling 13dba28 on stevejameskent:csv into e7391d1 on cfpb:milestone9.

LinuxBozo added a commit that referenced this pull request Apr 9, 2015
@LinuxBozo LinuxBozo merged commit 92c8cf5 into cfpb:milestone9 Apr 9, 2015
@stevejameskent stevejameskent deleted the csv branch April 10, 2015 17:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants