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

Added a range option to Worksheet.get_notes [Issue #1482] #1487

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

muddi900
Copy link
Contributor

Fixes #1482

@alifeee
Copy link
Collaborator

alifeee commented Jun 25, 2024

hi! thanks for the change! your code was all good, but the test cassette was breaking things. This is common, and what I usually do is completely delete the JSON test cassette and re-create it, effectively running:

rm tests\cassettes\WorksheetTest.test_get_notes.json
GS_CREDS_FILENAME="tests/creds.json"
GS_RECORD_MODE="all"
pytest -k "test_get_notes" -v -s tests/ 
# check it worked
GS_RECORD_MODE="none"
pytest -k "test_get_notes" -v -s tests/ 

I have reformatted the test a little so it is easier to read for me, and changed range to grid_range, as range is a python built-in keyword that we should not change.

Also, in testing, I noticed that get_notes did not work with worksheets that were not the primary sheet, so I have fixed this (see 16429af and 983dcb5 above) too.

again thanks for the change! I will let @lavigne958 read this and merge when happy with it :)

@nbwzx
Copy link
Contributor

nbwzx commented Jun 29, 2024

Also, in testing, I noticed that get_notes did not work with worksheets that were not the primary sheet, so I have fixed this (see 16429af and 983dcb5 above) too.

Wait is that wrong?

For example, there are three worksheets (Sheet1, Sheet2, Sheet3).

res["sheets"][self.index]["data"][0].get("rowData", [{}]) returns the notes of current worksheet.
self.spreadsheet.worksheets()[0].get_notes() get the notes of Sheet1
self.spreadsheet.worksheets()[1].get_notes() get the notes of Sheet2
self.spreadsheet.worksheets()[2].get_notes() get the notes of Sheet3

If you change self.index to 0, you can only get the notes of the primary sheet (Sheet1),
self.spreadsheet.worksheets()[0].get_notes() get the notes of Sheet1
self.spreadsheet.worksheets()[1].get_notes() get the notes of Sheet1
self.spreadsheet.worksheets()[2].get_notes() get the notes of Sheet1
how can you get the notes of other worksheets (Sheet2, Sheet3)?

… test)

now we can index the 0th sheet as we know that only one sheet will ever be returned (sheet titles are unique)
@alifeee
Copy link
Collaborator

alifeee commented Jun 29, 2024

Wait is that wrong?

As it turns out... yes and no! ;]

When you provide a range, i.e., here

https://github.com/muddi900/gspread/blob/d4a57746816ad097d56969cfab4c9c12a483dff6/gspread/worksheet.py#L2657-L2658

Then the returned results has only one sheet. Printing res, we get

> get_notes()
# res = 
{'sheets':
  [
    {'data': [{}]},
    {'data': [{'rowData': [{'values': [{'note': 'the first time'}]}, {}, {'values': [{}, {'note': 'two sheets'}]}]}]}
  ]
}
> get_notes("A2:C3")
# res = 
{'sheets':
  [
    {'data': [{'rowData': [{}, {'values': [{}, {'note': 'two sheets'}]}]}]}
  ]
}

So in the initial case, we do need to index it with worksheet.index, i.e, 1

In the second case, we need to use 0. I had spotted the latter case, which is why I made my change. However, I didn't make a very good test to be able to spot the reason it had changed.

I have made the test clearer (split it out) and 'fixed' it by always providing worksheet.title, so we can only ever expect one sheet in the response.

Thanks for spotting @nbwzx ! :)

@alifeee alifeee changed the title Added a range option to Worksheet.get_range [Issue #1482] Added a range option to Worksheet.get_notes [Issue #1482] Jun 29, 2024
@alifeee alifeee requested a review from lavigne958 June 29, 2024 15:16
@alifeee alifeee added this to the 6.2.0 milestone Jun 29, 2024
@alifeee
Copy link
Collaborator

alifeee commented Jun 29, 2024

sorry for jumping on your PR @muddi900. wouldn't have happened without you ;] so thanks for proposing the change!

will let @lavigne958 review and with an approval I will merge, and this can be included in 6.2.0 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

get_notes from a range
3 participants