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

Feature: add maxRows for #275 #277

Closed
wants to merge 1 commit into from

Conversation

cbrittingham
Copy link

@cbrittingham cbrittingham commented Aug 5, 2019

Link to feature description: #275

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

  1. Have you added tests for the new feature
    -> I looked at this briefly, but was not successful in finding how to unit test this properly.
  2. Does your submission pass tests?
  3. Have you lint your code locally prior to submission?
  4. Have you updated the docs?
    • If you added new parsing or formatting options have you added them to the docs?
    • If applicable have you added an example to the parsing or formatting docs?

@doug-martin
Copy link
Member

Ill take a look at this in the next day or two.

The one thing that sticks out to me is that the maxRows should probably be in the parserStream, which can bypass the parser all together, not sure how easy that would be though.

The other thing I think we should explore with this is can we short circuit the parsing once we hit the max rows emit and end event, Im not sure how well node streams would handle this though it might cause an error.

@cbrittingham
Copy link
Author

Ill take a look at this in the next day or two.

The one thing that sticks out to me is that the maxRows should probably be in the parserStream, which can bypass the parser all together, not sure how easy that would be though.

The other thing I think we should explore with this is can we short circuit the parsing once we hit the max rows emit and end event, Im not sure how well node streams would handle this though it might cause an error.

Do you have any suggestions on how I could short circuit to keep from reading the entire file without an enhancement? Is there anything I could do in my first "on data" callback?

@jgchristopher
Copy link

Can we push this out as is and add a new ticket to add the short circuit functionality?

doug-martin added a commit that referenced this pull request Dec 15, 2019
* [ADDED] `maxRows` option to limit the number of rows parsed. #275 #277
This was referenced Dec 15, 2019
@doug-martin doug-martin added this to the v3.5.1 milestone Dec 15, 2019
@doug-martin
Copy link
Member

This will be in v3.5.1

@doug-martin doug-martin mentioned this pull request Dec 16, 2019
doug-martin added a commit that referenced this pull request Dec 17, 2019
* [ADDED] `maxRows` option to limit the number of rows parsed. #275 #277 - @cbrittingham
* [ADDED] `skipRows` to allow skipping parsed rows see parsing.md
* [ADDED] `skipLines` to allow skipping entire lines of a csv parsing.md #267
* Exported formatting and parsing types.
* Removed `.npmignore` in favor of `package.json` files
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

3 participants