-
Notifications
You must be signed in to change notification settings - Fork 44
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
Implement FKSearch #131
base: jharkhand
Are you sure you want to change the base?
Implement FKSearch #131
Conversation
Implemented Fksearch on a sample data. Added a sample test and git expected output. Though need to figure out on how to apply in cqube dataset
@@ -11,7 +12,11 @@ export async function readCSV(filePath: string): Promise<string[][]> { | |||
.createReadStream(filePath) | |||
.pipe(csv({ separator: ',', headers: false, quote: "'" })) | |||
.on('data', (data) => { | |||
rows.push(Object.values(data)); | |||
const rowValues: string[] = Object.values(data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this doing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how is this logic ensuring that case insesitivity is achieved? What if the dimensions table has a value "CQUBE" and caseSensitiveFKSearch
flag is set to false
and then the CSV has cqube
, ideally as per the issue description this case should pass whereas in my understanding with the implementation in this PR, it will fail. Am I missing something here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, kindly change the base branch of this PR.
@@ -1,6 +1,7 @@ | |||
{ | |||
"globals": { | |||
"onlyCreateWhitelisted": true | |||
"onlyCreateWhitelisted": true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't let the onlyCreateWhiteList
changes come into this PR.
Pull Request Test Coverage Report for Build 5643721587Details
💛 - Coveralls |
Implemented Fksearch on data as part of C4GT issue. #63