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

Bugfix/get all record duplicated columns #1021

Merged
merged 3 commits into from Apr 12, 2022

Conversation

lavigne958
Copy link
Collaborator

Add a new argument to get_all_records to provide the list of expected
headers.

The given expected headers must:

  • be unique
  • be part of the complete headers list
  • must not contain extra headers

This will provide a way for users to use this method and still
have some duplicated headers that are not relevant to pull.

This will ensure the columns that matters have unique headers.

Closes #1007

Add a new argument to `get_all_records` to provide the list of expected
headers.

The given expected headers must:
- be unique
- be part of the complete headers list
- must not contain extra headers

This will provide a way for users to use this method and still
have *some* duplicated headers that are not relevant to pull.

This will ensure the columns that matters have unique headers.

Closes #1007
@lavigne958 lavigne958 self-assigned this Apr 6, 2022
Copy link

@robrap robrap left a comment

Choose a reason for hiding this comment

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

Thanks. This would be great. Added some comments for you to consider, but nothing blocking this.

gspread/worksheet.py Show resolved Hide resolved
gspread/worksheet.py Show resolved Hide resolved
gspread/worksheet.py Show resolved Hide resolved
Copy link

@robrap robrap left a comment

Choose a reason for hiding this comment

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

I forgot one more comment.

tests/worksheet_test.py Outdated Show resolved Hide resolved
Improved test coverage, make sure the logic is and remains.
Add note to users to expect all headers to be returned even if only a
subset is provided by `expected_headers`.
Updated test cassette on the way.
Clean old comment in tests when preparing sheet data.
@lavigne958
Copy link
Collaborator Author

Thanks. This would be great. Added some comments for you to consider, but nothing blocking this.

Thank you for your review, it is much appreciated.

You're right my first test scenario was too simple, I improved it to check the new logic and added a note to mention all the headers are returned.

@lavigne958
Copy link
Collaborator Author

I forgot one more comment.

you're right, this was probably an old comment and the code part was copied over and over with time. I cleaned it.

Copy link

@robrap robrap left a comment

Choose a reason for hiding this comment

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

Nice. Thanks. I added one more idea to consider for the future.

tests/worksheet_test.py Show resolved Hide resolved
@lavigne958 lavigne958 merged commit 610ad86 into master Apr 12, 2022
@lavigne958 lavigne958 deleted the bugfix/get_all_record_duplicated_columns branch April 12, 2022 14:51
@robrap
Copy link

robrap commented Apr 14, 2022

Also, in case this helps anyone, this is still a backward incompatible change for some. If you newly run into an issue, you’ll need to update your code to pass expected_headers with a list of any of the headers you expect. I’m not sure if sending an empty list would provide the legacy behavior.

@lavigne958
Copy link
Collaborator Author

I’m not sure if sending an empty list would provide the legacy behavior.

It will, it will be accepted as a valid list of expected headers, then all checks should pass.

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.

Duplicate header check in 5.2.0 is not backward compatible
3 participants