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

Many fixes for get_records #1357

Merged
merged 17 commits into from Dec 3, 2023
Merged

Conversation

alifeee
Copy link
Collaborator

@alifeee alifeee commented Nov 30, 2023

closes #590, #629, #1354, #1355

firstly, a test simplification

This is done for a few tests

...
- cell_list = self.sheet.range("A1:D4")
- for cell, value in zip(cell_list, itertools.chain(*rows)):
-   cell.value = value
- self.sheet.update_cells(cell_list
+ self.sheet.update("A1:D4", rows)
...

combine test_get_all_records_duplicate_keys and test_get_all_records_expected_headers

add test_get_all_records_with_blank_final_headers

Regression test for #1354

tests get_records with some blank final headers, i.e.,

"header 1" "header 2" "" ""

It should throw an error, but work fine with expected_headers set

add test_get_all_records_with_keys_blank

Similar to above but with the entire keys row blank

add test_get_records_with_all_values_blank

This is the beefy one. Regression test for #590, #629, #1355

Given the sheet

"a" "b" "c"
"" "" ""
"" "" ""

What should get_records return?

  • [] ?
  • [{"a": "", "b": "", "c": ""}, {"a": "", "b": "", "c": ""}] ?

This is a hard question. My answer uses the kwarg last_index. If this is asked for (i.e., if a user asks for "five rows"), then I believe get_records should return a list of length asked for (i.e., length 5). If it is not asked for (i.e., if a user asks for "entire sheet"), then I believe get_records should return an empty list (not further because we "don't know" how long a spreadsheet is). This logic is explained by:

I ask for get_records(first_index=2, last_index=4)

I want [{...}, {...}, {...}]

I ask for get_records()

I want []

I ask for get_records(first_index=1)

I want []

I ask for get_records(last_index=4)

I want [{...}, {...}, {...}]

Thus, this is the logic I included in test_get_records_with_all_values_blank

@lavigne958
Copy link
Collaborator

closes #590, #629, #1354, #1355

firstly, a test simplification

This is done for a few tests

...
- cell_list = self.sheet.range("A1:D4")
- for cell, value in zip(cell_list, itertools.chain(*rows)):
-   cell.value = value
- self.sheet.update_cells(cell_list
+ self.sheet.update("A1:D4", rows)
...

combine test_get_all_records_duplicate_keys and test_get_all_records_expected_headers

add test_get_all_records_with_blank_final_headers

Regression test for #1354

tests get_records with some blank final headers, i.e.,

"header 1" "header 2" "" ""
It should throw an error, but work fine with expected_headers set

add test_get_all_records_with_keys_blank

Similar to above but with the entire keys row blank

add test_get_records_with_all_values_blank

This is the beefy one. Regression test for #590, #629, #1355

Given the sheet

"a" "b" "c"
"" "" ""
"" "" ""
What should get_records return?

  • [] ?
  • [{"a": "", "b": "", "c": ""}, {"a": "", "b": "", "c": ""}] ?

To le we should return an empty list.
There is not data.

This is a hard question. My answer uses the kwarg last_index. If this is asked for (i.e., if a user asks for "five rows"), then I believe get_records should return a list of length asked for (i.e., length 5). If it is not asked for (i.e., if a user asks for "entire sheet"), then I believe get_records should return an empty list (not further because we "don't know" how long a spreadsheet is). This logic is explained by:

I ask for get_records(first_index=2, last_index=4)

I want [{...}, {...}, {...}]

I ask for get_records()

I want []

I ask for get_records(first_index=1)

I want []

I ask for get_records(last_index=4)

I want [{...}, {...}, {...}]

Thus, this is the logic I included in test_get_records_with_all_values_blank

Make sense if the user requests a range limit then get empty values with the requested size. Otherwise juste empty list.

I will take a look ASAP. By the end of the weekend it should be done.

Great job and thank you for taking it si fast.

@alifeee
Copy link
Collaborator Author

alifeee commented Dec 2, 2023

I will take a look ASAP. By the end of the weekend it should be done.

Super :) let me know if you think we should change anything

@lavigne958
Copy link
Collaborator

Looks good to me 👍

@alifeee alifeee merged commit 0cb6e2e into master Dec 3, 2023
12 checks passed
@alifeee alifeee deleted the 1354-get-records-with-blank-header branch December 3, 2023 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants