Added support for renaming duplicate headers#956
Conversation
|
Hi Thanks for creating the PR. As starting point we need to have a test case (not a test file). This means creating a function on I have some concerns about your code:
Could you please address such issues? Thanks |
|
Thanks for the quick feedback & sorry for the misunderstanding re: test cases. I've added a test case, removed the useless comments, and added a conditional based on whether the header is present. You were right - the renaming code could be refactored. I originally just took the code from that issue thread which worked but was inefficient. I've rewritten it in a simpler way which produces the same result. |
There was a problem hiding this comment.
Thanks for your work on this. Everything looks nice, I just added some comments to improve code redability and ensure everything works as expected. Could you please review them?
|
Have addressed all comments re: code-cleanup. Good call on the transformHeader() test. I added it and It originally failed. I added a check for the function when initialising the header in my code. Hopefully this is satisfactory. I also added an extra test in the case of an existing header matching an intended substitution. An unlikely situation but thought it'd be wise to cover it. |
There was a problem hiding this comment.
Everythinkg looks nice now. I leaved some minor issues that should be addressed.
Could you fix them? I think we will be able to merge it once they are addressed.
Thanks for your time on this.
papaparse.js
Outdated
| header = config.transformHeader(header, i); | ||
| var header_name = header | ||
| if (header_map.includes(header_name)){ | ||
| if (!(header in header_count)){ |
There was a problem hiding this comment.
I think its simpler to write as:
count = header_count[header_name] || '1';
header_name = header+separator+count;
header_count[header_name] = count +1;
Removeing the if else lines.
There was a problem hiding this comment.
I struggled to make it work without an if statement. The problem being that it would still add the suffix at the end. The best I got was:
let count = header_count[header] || 0
if (count > 0) header_name = header+separator+count;
header_count[header] = count+1
Have committed this for you consideration if you prefer.
|
@fortydegrees linting is failing, could you have a look? https://github.com/mholt/PapaParse/actions/runs/3463298598/jobs/5783355405 |
Should be fixed now. Thank you for your excellent help in cleaning everything up! |
|
@fortydegrees I've merged your work. Thanks you so much! |
|
Thank you for resolving this @fortydegrees! @pokoli Would you mind doing another minor version bump so we can use these changes in production? |
Added code to automatically rename headers to solve the issue in #129
I tried to add a renameDuplicateHeaders config option but left it out as I am doing the processing in the Parse.parse function where I don't have the ability to read the config. I'm not sure it fits the styling of the codebase to modify the input outside of that function so have left it out.