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/update-internal-properties #1225

Merged
merged 14 commits into from
Jun 16, 2023
Merged

Fix/update-internal-properties #1225

merged 14 commits into from
Jun 16, 2023

Conversation

alifeee
Copy link
Collaborator

@alifeee alifeee commented Jun 16, 2023

Changes discussed in #881

gspread.Worksheet._properties (and thus the gspread.Worksheet @property fields) are now updated whenever the following are called (as well as previous changes):

  • append_rows
  • insert_rows
  • insert_cols
  • delete_dimension

(as you can see, the leftover was mostly gspread.Worksheet.row_count and [...].col_count

also new tests added for insert_cols and delete_cols (before, only row tests existed)

@alifeee alifeee marked this pull request as draft June 16, 2023 00:44
@alifeee
Copy link
Collaborator Author

alifeee commented Jun 16, 2023

Final thoughts: we should consider if gspread.Spreadsheet has any internal properties that are not updated, in a similar vein. On a brief look, there are:

  • id
  • title
  • url
  • creationTime
  • lastUpdateTime
  • updated
  • timeZone
  • locale
  • sheet1

These are not updated on property change. I see:

  • id, creationTime are never updated (or shouldn't be?)
  • title, timeZone, locale should be easy fixes (I will do these in Fix/update-internal-properties #1225)
  • I have not looked at the implementation of lastUpdateTime, but it seems like it would be complex to update. Perhaps an extra API call makes sense for this one (but then as a method rather than a @property - deprecate the property?)
  • sheet1, url, updated do not directly depend on _properties so can be ignored

@alifeee
Copy link
Collaborator Author

alifeee commented Jun 16, 2023

(based on comment #881 (comment))

closes #881

@alifeee
Copy link
Collaborator Author

alifeee commented Jun 16, 2023

looks the only methods we may have to edit are

  • gspread.Spreadsheet.update_title
  • gspread.Spreadsheet.update_timezone
  • gspread.Spreadsheet.update_locale

I will do this soon.

and then the discussion of lastUpdateTime above - require @lavigne958 thoughts.

@alifeee
Copy link
Collaborator Author

alifeee commented Jun 16, 2023

ok, done title, timezone, & locale

it is ready to merge now, just after wondering whether lastUpdatedTime should change. Current implementation:

@property
def lastUpdateTime(self):
"""Spreadsheet last updated time."""
try:
return self._properties["modifiedTime"]
except KeyError:
metadata = self.client._get_file_drive_metadata(self.id)
self._properties.update(metadata)
return self._properties["modifiedTime"]

So it runs an API request if it is not already in _properties. Due to the nature of the property, it might be nice to run the API request every time, so that it is always up to date. However, this may be better suited to a function, i.e. getLastUpdatedTime(), so it's more obvious the API is used. Thoughts?

@alifeee alifeee marked this pull request as ready for review June 16, 2023 10:53
@alifeee alifeee requested a review from lavigne958 June 16, 2023 10:53
@lavigne958
Copy link
Collaborator

That's hard to say, currently we request it if not present but as you mention it's not updated.

To me we could add a new method like you said getLastUpdatedTime but instead of get we coudl make it actually update the last update time, kind of like retrieve the last update time. this makes it obvious that the method makes an API call then the user still uses the property to get it.

In a way we provide a way to update the property.

does that makes sense ? this way we can add it to the next release using the current PR without breaking anything.

@alifeee alifeee merged commit c38c512 into master Jun 16, 2023
@alifeee alifeee deleted the fix/update-internal-properties branch June 16, 2023 22:06
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.

2 participants