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

fix: avoid strict validations for skipped rows #138

Closed
wants to merge 2 commits into from

Conversation

RimashMohomed
Copy link

@RimashMohomed RimashMohomed commented Jul 13, 2019

This PR contains:

  • bugfix
  • feature
  • refactor
  • tests
  • documentation
  • metadata

Breaking Changes?

  • yes
  • no

skipped rows skipped using skipLines are not validated against strict option

fixes issue #136

@RimashMohomed
Copy link
Author

FYI @shellscape

@shellscape
Copy link
Collaborator

shellscape commented Jul 16, 2019

Please don't tag/ping maintainers in replies unless a significant amount of time has passed without review.

Thanks for the PR 🍺 You'll need to add a test for this fix for us to accept the changes.

@RimashMohomed
Copy link
Author

RimashMohomed commented Jul 18, 2019

Please don't tag/ping maintainers in replies unless a significant amount of time has passed without review.

Thanks for the PR beer You'll need to add a test for this fix for us to accept the changes.

sure will add tests and re submit, anyway contribution link is broken such i couldn't find the requirements for a PR, can you please provide a resource for the completeness criteria for a PR (to avoid the trouble)?

@shellscape
Copy link
Collaborator

No need to resubmit the PR, you need only to commit the tests to your fork and the PR will pick it up automatically. Here is the contributing file: https://github.com/mafintosh/csv-parser/blob/master/.github/CONTRIBUTING.md. It's generally considered good practice to update or add tests when submitting a fix or new feature.

@shellscape
Copy link
Collaborator

Thanks for putting together this PR. It is appreciated. The fix for this was somewhat simpler than what you had submitted. I've committed a fix and tests for it. We'll release this fix later on in the day.

@shellscape shellscape closed this Sep 24, 2019
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.

2 participants