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

Avoid doing i/o in properties #1253

Closed
dgilman opened this issue Jul 9, 2023 · 3 comments · Fixed by #1255
Closed

Avoid doing i/o in properties #1253

dgilman opened this issue Jul 9, 2023 · 3 comments · Fixed by #1255
Assignees
Milestone

Comments

@dgilman
Copy link
Contributor

dgilman commented Jul 9, 2023

Describe the bug
There are a handful of @property methods on the Spreadsheet object that do i/o under the hood. This isn't a great idea for all the same reasons that were listed in #600. There are a bunch of ways forward here but it seems sensible to introduce new methods as replacements, depreciate the current properties, and eventually remove the properties in gspread 6.x.

Properties:

  • creationTime
  • lastUpdateTime
  • sheet1
@alifeee
Copy link
Collaborator

alifeee commented Jul 9, 2023

Hi, thanks for the suggestion :)

This is something that I've been thinking about a lot recently. I agree with you. I would not want my @property accesses to send API requests, as I wouldn't expect this.

At least for creationTime and lastUpdateTime, this request is only sent once (if it does not exist), then not afterwards.

For sheet1, it calls every time. I agree this is not great usability. sheet1 is a very common method in gspread though. BUT, the spreadsheet id 0 should exist when the Spreadsheet object is created.

Related issues: #600, #1227

Related to @property methods (and not being newest when they are updated): #1201, #881, #1225, #1212

We will look at this for version 6 :)

@alifeee alifeee added this to the 6.0.0 milestone Jul 9, 2023
@lavigne958
Copy link
Collaborator

Hi I remember this issue.

I agree we should update that process.

We can merge deprecation warning now in release 5.11, then in release 6.x change all @property occurrences and replace them with a function of the same name. It makes more sense.

To keep in mind we can't remove the I/O when getting a sheet, for the matter that the time someone opens a spreadsheet and the time that user opens the first sheet is not always the same. Getting the latest metadata for a worksheet when opening it is good practice to me. Otherwise most people would get it then call fetch_sheet_metadata anyway.

@alifeee alifeee modified the milestones: 6.0.0, 5.11.0 Jul 10, 2023
@alifeee alifeee self-assigned this Aug 8, 2023
@alifeee
Copy link
Collaborator

alifeee commented Aug 8, 2023

See PR above.

I have not changed sheet1's behaviour, as per the discussion above. Thus, after #1255 is completed, I consider this issue completed.

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 a pull request may close this issue.

3 participants