add get_notes

[nbwzx]

get_note can only get the content of the note located at one cell, we need get_notes to get all notes in the sheet

[lavigne958]

Hi @nbwzx thank you for this feature this is realy nice !

I have a few comments on how you solve this issue. once solved we can merge it.

[nbwzx]

I have updated based on your suggestions. Thanks for your detailed comments!

[lavigne958]

tried it, works like a charm, what is left to do:

• documentation
• typing

then we can merge.

[nbwzx]

I have added the documentation and typing, thanks.

[lavigne958]

Thank you for the feature.

Please rebase your code on the master branch then we can run the CI to check/test your code.

[nbwzx]

Unfortunately, tox cannot be installed on my computer because of the dependency conflicts. I have fixed the missing typing from the workflow.

[lavigne958]

great thank you for this contribution.

[alifeee]

I think a test should have been added before merging this PR. My apologies for not suggesting this sooner, but I did not expect it to be merged when it was, so I did not comment yet. :)

I have made such a test in the test/get-notes branch -> https://github.com/burnash/gspread/compare/master..test/get-notes

This function also raises a question of default behaviour: should get_notes return a square array or a non-square array?

non-square arrays are unexpected from users. See #1289 & #1305 for much more information on this, but effectively, which of the following structures should a user expect?

[
  ["top left note", None],
  [None, "bottom right note"],
]

[
  ["top left note"],
  [None, "bottom right note"],
]

Whichever we think makes the most sense should be represented in a test, so that behaviour is maintained.

Finally, I think this is very wordy

    .. note::
       The resulting matrix is as long as the last row holding a note
           (could be smaller than the last data row)
       The resulting matrix is as large as the last column holding a note
           (could be smaller than the last data column)

and would better be represented with an example like provided above.

As @lavigne958 says, thank you for the work on this ! :)

[nbwzx]

I think a test should have been added before merging this PR. My apologies for not suggesting this sooner, but I did not expect it to be merged when it was, so I did not comment yet. :)

I have made such a test in the test/get-notes branch -> https://github.com/burnash/gspread/compare/master..test/get-notes

Yeah, that's pretty good.

This function also raises a question of default behaviour: should get_notes return a square array or a non-square array?

non-square arrays are unexpected from users. See #1289 & #1305 for much more information on this, but effectively, which of the following structures should a user expect?

[
  ["top left note", None],
  [None, "bottom right note"],
]

[
  ["top left note"],
  [None, "bottom right note"],
]

Whichever we think makes the most sense should be represented in a test, so that behaviour is maintained.

I think a square array would be better after I reading your links. Also, it makes the output of get_values and get_notes correspond to each other, which is I expected.

By the way, shall we add some parameters of get_values into get_notes, e.g. range_name, maintain_size?

[alifeee]

I think a square array would be better after I reading your links. Also, it makes the output of get_values and get_notes correspond to each other, which is I expected.

Technically, for historical reasons both cases (square and not) are "default". See #1296 (comment) for context, but effectively, get_values and get are the same function, but have different defaults, so with

"this is "	-
-	"some text"

and

print("worksheet.get")
print(worksheet.get("A1:B2"))
print("worksheet.get_values")
print(worksheet.get_values("A1:B2"))

you get

worksheet.get
[
  ['this is'],
  ['', 'some text']
]
worksheet.get_values
[
  ['this is', ''],
  ['', 'some text']
]

So... I am not sure which default this function should have, or whether it should have maintain_size kwargs. My preference is to keep it as simple as possible (so leave it as is), and add a note that users can use utils.fill_gaps themselves if they want a square array. For example, with utils.fill_gaps(arr, len(arr), max(len(a) for a in arr)).

By the way, shall we add some parameters of get_values into get_notes, e.g. range_name, maintain_size?

I don't think so, because adding utilities and kwargs makes things complicated and hard to maintain. For example, get_all_values got out of hand before -> #1367, so I think keeping this simple works best.

I do think if there are no notes, get_notes should return [[]] instead of [] as this is what worksheet.get does.

Run

print(worksheet.get_notes())
print(worksheet.get("H2:I3"))

on an empty sheet to get

[]
[[]]

(I say this also partly because the above fill_gaps example works on [[]], but errors on [])

[nbwzx]

Thank you for the explanation.
I agree with these changes:

1. add test for get_notes
2. add an example in documentation
3. get_notes return [[]] instead of [] if there are no notes

You have already finished (1) in test/get-notes, I don't know how can I help with others since it is a merged pr. Shall I open a new pr or is it convenient for you to update them?

[alifeee]

if you'd like to contribute, you can pull the branch with the test and make a new PR.

Thanks for the enthusiasm!

[ileodo]

@alifeee @nbwzx I understand this is closed, just wondering why not returning Mapping[str, str] (key by a1, value by note content) . As it will reduce the confusion of introducing matrix, is there any other api returning matrixes?

[lavigne958]

@alifeee @nbwzx I understand this is closed, just wondering why not returning Mapping[str, str] (key by a1, value by note content) . As it will reduce the confusion of introducing matrix, is there any other api returning matrixes?

Hi when using gspread most data you get is in a matrix format (or a single list format when requesting a single row) so the general usage is to iterate over a matrix.

Iteration over a dict is not standard, the only method providing a dict result is get_all_records which returns a list of dict due to parsing of headers.

In this way we agree to return a matrix (list of list).

[twosheds]

I think get_notes would be more useful if if could be applied to a range.

[alifeee]

I think get_notes would be more useful if if could be applied to a range.

Hi @twosheds, thanks for the suggestion! Since this PR is closed, could you instead open an issue? thanks