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

Simplify get records #1374

Merged
merged 2 commits into from
Jan 9, 2024
Merged

Simplify get records #1374

merged 2 commits into from
Jan 9, 2024

Conversation

alifeee
Copy link
Collaborator

@alifeee alifeee commented Dec 29, 2023

closes #1367

For discussion, see #1367

I made the proposed changes, apart from

  • did not rename expected_headers
    I think it will be easier for all if this stays. I only see it potentially being annoying compared to ignore_duplicate_headers if the headers are unknown, in which case we can tell the user to use worksheet.get("1:1") to fetch the headers, to use in expected_headers
  • did not remove empty2zero
    it was used in numericization, I moved it under the numericization kwarg to reflect the relation

And:

I did not implement utils.to_records, as @lavigne958 expressed desire to do this

see #1367
rearrange kwargs and docstring kwargs
this simplifies the method a lot (as discussed in #1367)

had to change some tests
- replace get_records with get_all_records
- remove testing logic that use `first_index` and `last_index`. these are no longer supported
@alifeee alifeee added this to the 6.0.0 milestone Dec 29, 2023
@alifeee alifeee self-assigned this Dec 29, 2023
@alifeee alifeee changed the base branch from master to feature/release_6_0_0 December 29, 2023 18:49
@alifeee alifeee merged commit 63c4c5e into feature/release_6_0_0 Jan 9, 2024
10 checks passed
@alifeee alifeee deleted the simplify-get-records branch January 9, 2024 15:18
@alifeee alifeee mentioned this pull request Jan 26, 2024
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.

PROPOSAL: changes to get_records/get_all_records
2 participants