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

Add line offset headers #42

Merged
merged 4 commits into from Feb 23, 2022
Merged

Add line offset headers #42

merged 4 commits into from Feb 23, 2022

Conversation

emorydunn
Copy link

@emorydunn emorydunn commented Oct 4, 2021

Useful for skipping rows in CSVs that have table titles

Description

This PR adds a header strategy to parse headers from the specified row number, ignoring any previous row (and fixes a few typos). The change is additive, leaving the existing .firstLine option in place. I've been using the the header strategy in of of my own apps without any issues.

The tests have been updated to include the new strategy by way of a table title, and all pass.

Checklist

The following list must only be fulfilled by code-changing PRs. If you are making changes on the documentation, ignore these.

  • Include in-code documentation at the top of the property/function/structure/class (if necessary).
  • Merge to develop.
  • Add to existing tests or create new tests (if necessary).

Useful for skipping rows in CSVs that have table titles
@emorydunn
Copy link
Author

@dehesa I just wanted to check in and see if there's anything else I need to do with the PR for it to be merged in. Thanks!

@dehesa
Copy link
Owner

dehesa commented Feb 17, 2022

Hi @emorydunn

First of all, I am very sorry I haven't looked into this 😔 My focus was on other projects and I didn't really noticed. I will check this week the PR and leave you some comments or accept it.

In any case, I highly appreciate you putting in the time to contribute here.

Copy link
Owner

@dehesa dehesa left a comment

Choose a reason for hiding this comment

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

Hey @emorydunn ,

Thank you for the contribution and the grammar changes. The PR looks good overall, I just left you some small comments. Also, please be sure to target the merge on the develop branch.

sources/imperative/reader/Reader.swift Outdated Show resolved Hide resolved
sources/imperative/reader/Reader.swift Outdated Show resolved Hide resolved
sources/imperative/reader/Reader.swift Outdated Show resolved Hide resolved
@emorydunn emorydunn changed the base branch from master to develop February 21, 2022 17:48
@emorydunn
Copy link
Author

Thanks for taking the time to review everything. I updated the rowIndex in Reader.swift and think I found all of the indentation issues.

Copy link
Owner

@dehesa dehesa left a comment

Choose a reason for hiding this comment

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

Thanks for the changes @emorydunn. I just left a final comment.

sources/imperative/reader/Reader.swift Outdated Show resolved Hide resolved
@dehesa dehesa merged commit 71e845e into dehesa:develop Feb 23, 2022
@dehesa
Copy link
Owner

dehesa commented Feb 23, 2022

Done and, again, I am sorry it took so long.

@emorydunn
Copy link
Author

Thanks, and don't worry about the timing, I'm happy to be able to contribute to a library I use so much

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