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

PROPOSAL: changes to get_records/get_all_records #1367

Closed
alifeee opened this issue Dec 7, 2023 · 3 comments · Fixed by #1374
Closed

PROPOSAL: changes to get_records/get_all_records #1367

alifeee opened this issue Dec 7, 2023 · 3 comments · Fixed by #1374
Assignees
Labels
Need investigation This issue needs to be tested or investigated
Milestone

Comments

@alifeee
Copy link
Collaborator

alifeee commented Dec 7, 2023

get_all_records is a method which has provided much pain to maintain recently (issues #1363, #1355, #1354, #1352, #1294) (PRs #1364, #1357, #1345, #1343, #1301, #1028)

We propose that we simplify the method, and reduce its scope.

New scope

Given a spreadsheet, get_all_records turns a header and following content into a list of records

API information

The API call will return to the pre-5.12 case:

data = self.get_all_values(value_render_option=value_render_option)

Even if the header is set to the 5th row, we still need to get all values, as we do not know the "final row" so we cannot use a row range like "5:999"

my sheet
fruit price
apple 56
orange 23
> get_all_records(header=2)
[
  {"fruit": "apple", "price": "56"},
  {"fruit": "orange", "price": "23"},
]

Features to be changed

Simplify the "duplicate headers" case

- get_all_records(expected_headers=["apple", "orange"])
+ get_all_records(allow_duplicate_headers=True)

Features to be removed

get_records method, i.e., range_specification.

This means that headers + data will be obtained in a single API call. This simplifies a lot since we no longer have to pad the header/values to be the same length (#1357), as they will be the same length by nature of being from the same API call.

- get_records(first_index=10, last_index=15)

empty2zero kwarg.

Its job can be done by existing default_blank

Details

Technically these differ currently. default_blank is used to replace a single blank header (not more), and also replace blank values. However, I think it should change to only replace values. Blank header should be left as-is and if there are multiple, duplicate headers can be ignored (see above).

- get_all_records(empty2zero=True)
+ get_all_records(default_blank=0)

Features to be added

utils.to_records

Add a utils function to turn a list of headers + values into a list of dictionaries

implementation details

this will basically be a wrapper of

formatted_records = [dict(zip(keys, row)) for row in values]

+ utils.to_records(header, values)

This can be put into documentation to tell people how to make their own list of records, given a header and values.

+ Want to use get_records differently? You can make your own by using `utils.to_records`:
+ header = worksheet.get("4:4")
+ values = worksheet.get("20:40")
+ records = utils.to_records(header, values)
@lavigne958
Copy link
Collaborator

Should we get started on this ?

I propose:

  • I make the new function utils.to_records
  • you change the signature of get_all_reccords to accomodate to the new changes, with the necessary code that changes due to removal of input parameters etc
  • you remove the method get_records
  • we merge both PRs in feature/release_6_0_0, both could be done in parallel I believe.

after that then:

  • I update the code of get_all_records to use utils.to_records
  • we make sure the tests are still valid with no changes to the cassettes nor the tests (may be the test will need adjustment: new inputs, renmed inputs at most).

What do you think ?
From there when it works:

  • we update the README
  • we release 6.0.0 ? 🙄

@alifeee
Copy link
Collaborator Author

alifeee commented Dec 27, 2023

sounds good to me :)

let me read what I wrote 3 weeks ago to remember what we're doing... ;)

I should make edits to get_all_records on a branch against master or feature_release_6_0_0?

@lavigne958
Copy link
Collaborator

sounds good to me :)

let me read what I wrote 3 weeks ago to remember what we're doing... ;)

I should make edits to get_all_records on a branch against master or feature_release_6_0_0?

you should start you branch from feature/release_6_0_0 and target that branch too on you PR.

So far to me we have everything for v5, we don't change anything anymore.

We need to apply the new changes on v6.0.0 for the method get_all_records then we are good for the first release 🙈

alifeee added a commit that referenced this issue Dec 29, 2023
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
lavigne958 added a commit that referenced this issue Jan 15, 2024
Add a new util function that helps building record values.
Records are list of dictionaries, each dictionary has the same
set of keys, they are set from the given list of headers.
Each key is associated to a value from the given matrix of values.
They are as many dictionaries as they are lines in the matrix.

Each line in the matrix will be associated to a dictionary,
each key is associated to a value from that line in the given order.

Example:
Headers: A B C
Values:
1 2 3
7 8 9
Result: [ { A: 1, B: 2, C: 3}, {A: 7, B: 8, C: 9}]

closes #1367

Signed-off-by: Alexandre Lavigne <[email protected]>
lavigne958 added a commit that referenced this issue Jan 15, 2024
Add a new util function that helps building record values.
Records are list of dictionaries, each dictionary has the same
set of keys, they are set from the given list of headers.
Each key is associated to a value from the given matrix of values.
They are as many dictionaries as they are lines in the matrix.

Each line in the matrix will be associated to a dictionary,
each key is associated to a value from that line in the given order.

Example:
Headers: A B C
Values:
1 2 3
7 8 9
Result: [ { A: 1, B: 2, C: 3}, {A: 7, B: 8, C: 9}]

closes #1367

Signed-off-by: Alexandre Lavigne <[email protected]>
lavigne958 added a commit that referenced this issue Jan 15, 2024
Add a new util function that helps building record values.
Records are list of dictionaries, each dictionary has the same
set of keys, they are set from the given list of headers.
Each key is associated to a value from the given matrix of values.
They are as many dictionaries as they are lines in the matrix.

Each line in the matrix will be associated to a dictionary,
each key is associated to a value from that line in the given order.

Example:
Headers: A B C
Values:
1 2 3
7 8 9
Result: [ { A: 1, B: 2, C: 3}, {A: 7, B: 8, C: 9}]

closes #1367

Signed-off-by: Alexandre Lavigne <[email protected]>
lavigne958 added a commit that referenced this issue Jan 15, 2024
Add a new util function that helps building record values.
Records are list of dictionaries, each dictionary has the same
set of keys, they are set from the given list of headers.
Each key is associated to a value from the given matrix of values.
They are as many dictionaries as they are lines in the matrix.

Each line in the matrix will be associated to a dictionary,
each key is associated to a value from that line in the given order.

Example:
Headers: A B C
Values:
1 2 3
7 8 9
Result: [ { A: 1, B: 2, C: 3}, {A: 7, B: 8, C: 9}]

closes #1367

Signed-off-by: Alexandre Lavigne <[email protected]>
lavigne958 added a commit that referenced this issue Jan 15, 2024
Add a new util function that helps building record values.
Records are list of dictionaries, each dictionary has the same
set of keys, they are set from the given list of headers.
Each key is associated to a value from the given matrix of values.
They are as many dictionaries as they are lines in the matrix.

Each line in the matrix will be associated to a dictionary,
each key is associated to a value from that line in the given order.

Example:
Headers: A B C
Values:
1 2 3
7 8 9
Result: [ { A: 1, B: 2, C: 3}, {A: 7, B: 8, C: 9}]

closes #1367

Signed-off-by: Alexandre Lavigne <[email protected]>
lavigne958 added a commit that referenced this issue Jan 19, 2024
Add a new util function that helps building record values.
Records are list of dictionaries, each dictionary has the same
set of keys, they are set from the given list of headers.
Each key is associated to a value from the given matrix of values.
They are as many dictionaries as they are lines in the matrix.

Each line in the matrix will be associated to a dictionary,
each key is associated to a value from that line in the given order.

Example:
Headers: A B C
Values:
1 2 3
7 8 9
Result: [ { A: 1, B: 2, C: 3}, {A: 7, B: 8, C: 9}]

closes #1367

Signed-off-by: Alexandre Lavigne <[email protected]>
lavigne958 added a commit that referenced this issue Jan 19, 2024
Add a new util function that helps building record values.
Records are list of dictionaries, each dictionary has the same
set of keys, they are set from the given list of headers.
Each key is associated to a value from the given matrix of values.
They are as many dictionaries as they are lines in the matrix.

Each line in the matrix will be associated to a dictionary,
each key is associated to a value from that line in the given order.

Example:
Headers: A B C
Values:
1 2 3
7 8 9
Result: [ { A: 1, B: 2, C: 3}, {A: 7, B: 8, C: 9}]

closes #1367

Signed-off-by: Alexandre Lavigne <[email protected]>
@alifeee alifeee unpinned this issue Jan 28, 2024
@alifeee alifeee mentioned this issue Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Need investigation This issue needs to be tested or investigated
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants