Skip to content
This repository has been archived by the owner on Sep 26, 2023. It is now read-only.

Port CSV Generation into master #105

Closed
wants to merge 2 commits into from
Closed

Port CSV Generation into master #105

wants to merge 2 commits into from

Conversation

marshyonline
Copy link
Contributor

@kernelogic has a nice branch that generates a CSV for a replication request that is useful for offline deal importing.
The current branch looks to have to many changes than necessary, so I have striped down the core function of this branch, changed where the csv is stored and some logic around creating the storage dir.

All Credit to @kernelogic for this one, I just tidied it up for master

Copy link
Contributor

@liuziba liuziba left a comment

Choose a reason for hiding this comment

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

@@ -10,6 +11,9 @@ import GetReplicationDetailsResponse from './GetReplicationDetailsResponse';
import { GetReplicationsResponse } from './GetReplicationsResponse';
import UpdateReplicationRequest from './UpdateReplicationRequest';
import { ObjectId } from 'mongodb';
import fs from 'fs-extra';
import path from 'path';
const ObjectsToCsv: any = require('objects-to-csv');// no TS support
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add @types/objects-to-csv as dev dependency so we get TS support

});
}
const csv = new ObjectsToCsv(csvRow);
const configDir = config.util.getEnv('NODE_CONFIG_DIR');
Copy link
Contributor

Choose a reason for hiding this comment

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

output dir should not be hardcoded here. Either, the request support an output directory/filename, or the response should contain the content of CSV so that the caller can decide which file to save the csv to.

@kernelogic
Copy link
Contributor

@marshyonline do you mind if I close this PR? I have merged my csv branch to master. #106

@marshyonline
Copy link
Contributor Author

Thanks Guys!

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.

None yet

3 participants