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

FIX: updating Worksheet properties after calling methods that can change them #548

Closed
wants to merge 2 commits into from

Conversation

crash5936
Copy link

Digging into issue #545 I found out, that there were multiple problems caused by this.
Essentially, when any method that modifies the properties of a sheet online is called, the Worksheet instance properties didn't get updated accordingly.

For example, when you called gspread.models.Worksheet.add_cols(x) twice in a row, it only added this number of columns to the initial number of columns (ie. the number of columns from when the Worksheet() class was instantiated)

I tried fixing this by adding a decorator, which causes the function to read the actual metadata from the API and update the instance properties accordingly - testing this, it seems to fix all the issues.

@aiguofer
Copy link
Collaborator

Nice catch, I encountered an issue caused by the same problem. The only concern with this approach is that it might increase the number of API calls significantly.

@aiguofer
Copy link
Collaborator

I got to thinking about this and got an idea. Instead of calling fetch_sheet_metadata each time we update a property on a worksheet, we hold the sheets_metadata in the Spreadsheet instance and add a flag to indicate that it needs updating. When accessing the sheets metadata, we can update it if it needs it, and each Worksheet's properties are a reference to the Spreadsheet, so they would be updated accordingly.

I'm not entirely sure how much this would actually reduce the number of calls for most usecases, but it seems like it could save some calls when working with multiple worksheets from the same spreadsheet.

see aiguofer@21c87dd (forked off your branch)

@lavigne958
Copy link
Collaborator

Hi this is a great idea. and it will prevent some issues for sure. thank you @crash5936.

As you mentioned @aiguofer, this will increase the number of calls toward the API.

We must think this through and find a solution that brings: robustness (the local metadata are up to date) and efficiency (we don't do more HTTP call toward the API than necessary).

I think at some points we won't have a choice, the metadata might change (some user change the column number using a web browser) and we are not up to date.

A solution could be to offer the user a choice:

  • Do not fetch metadata every times, rely on what we know locally, the script is the only one updating the spreadSheet
  • Get metadata every times after an update to make sure we are up to date, some else might be updating the spreadSheet outside the script (this could benefit from your idea @aiguofer to prevent having too many requests)

@alifeee
Copy link
Collaborator

alifeee commented Jun 8, 2023

fixed by #1211?

@lavigne958
Copy link
Collaborator

yes closed by #1211

this was one way to make it happen too but it's less specific. so far we can continue with our current method if we have so many properties to update at some point we'll change it for something like this.

@lavigne958 lavigne958 closed this Jun 14, 2023
@alifeee
Copy link
Collaborator

alifeee commented Jun 14, 2023

yes. this way also increased the API calls.

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.

4 participants