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

Implement https://github.com/mholt/PapaParse/issues/612 #898

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

theLAZYmd
Copy link

@theLAZYmd theLAZYmd commented Nov 3, 2021

  • Modify transformHeader to take 3 new parameters:
    -- The whole headers row as an array (arr)
    -- The previous value of the header in that column (acc)
    -- The row number (j)
  • Applies the transformHeader function over each row in data up to your headerLines value
  • Creates a config.headerLines parameter (default 1)

@pokoli
Copy link
Collaborator

pokoli commented Nov 4, 2021

This is related to #612

@pokoli
Copy link
Collaborator

pokoli commented Nov 4, 2021

Please do not update the minified file, this will be done by our release process.
Could you please update the review to revert such changes.

@pokoli pokoli self-requested a review November 4, 2021 13:39
@@ -510,8 +510,20 @@ <h5 id="config-details">Config Options</h5>
<code>transformHeader</code>
</td>
<td>
A function to apply on each header. Requires <code>header</code> to be <code>true</code>. The function receives the header as its first argument and the index as second.<br>
Only available starting with version 5.0.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should keep that this is only availalbe on the version 5.0

I also plan to do a minor version for the parameter. So the new parameter structure should be only available on 5.4+ versions. That should be noted too.

Copy link
Author

Choose a reason for hiding this comment

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

Resolved with ac6f191

}
// if _results.data[0] is not an array, we are in a step where _results.data is the row.
else
_results.data.forEach(addHeader);
_results.data.forEach(addHeader.bind(null, 0));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is bind required? Could we just ignore make the arguments at the end and optional?

Copy link
Author

Choose a reason for hiding this comment

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

Bind is required because we're trying to parse a function into the .forEach() method, and unfortunately we have
"no-loop-func": "error"
and ecma5 versioning in our eslint config, which I think means no arrow functions. I assume this is for reasons of universalisable compatibility and speed. As a result, this is the only way to pass the loop iterator into the function.

@theLAZYmd
Copy link
Author

Please do not update the minified file, this will be done by our release process. Could you please update the review to revert such changes.

Resolved with 417982c

@theLAZYmd
Copy link
Author

@pokoli and else, is there any update on this?

@pokoli
Copy link
Collaborator

pokoli commented Dec 10, 2021

We need a bug fix release before merging this.
I will take care of that!

@theLAZYmd theLAZYmd requested a review from pokoli January 7, 2022 10:55
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.

None yet

2 participants