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

Bugfix/add set timeout #1417

Merged
merged 1 commit into from
Mar 1, 2024
Merged

Bugfix/add set timeout #1417

merged 1 commit into from
Mar 1, 2024

Conversation

lavigne958
Copy link
Collaborator

Add missing method to set the HTTP Timeout.

it got lost when moving HTTP requests to new HTTP Client.

Now the timeout can be:

  • set using gspread client (like before, backward compatible)
  • from the spreadsheet object: spreadsheet.set_timeout()
  • from the sheet object: sheet.set_timeout()
  • from the HTTP client anywhere it can be accessed:
    • from gspread service: gs_client.http_client.set_timeout()
    • from the spreadsheet: spreadsheet.client.set_timeout()
    • from the sheet: sheet.client.set_timeout()

closes #1393

Copy link
Collaborator

@alifeee alifeee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

was this method in Worksheet and Spreadsheet before? if not, why do we add it? it is not needed for backwards compatibility

The tests should be updated to reflect this change. We added a test for the missing client methods if I remember

gspread/http_client.py Outdated Show resolved Hide resolved
@lavigne958
Copy link
Collaborator Author

lavigne958 commented Feb 21, 2024

looks good

was this method in Worksheet and Spreadsheet before? if not, why do we add it? it is not needed for backwards compatibility

not it was not, I thought I add extra ways to set timeout for users so it offer more entry point to set the timeout.
agreed it's not needed for backward compatibility.

The tests should be updated to reflect this change. We added a test for the missing client methods if I remember

I will check if I can add that, may be a new test that set a low timeout, so one day if we move it the test will fail when trying to call a method that does not exists, do you think this will be enough ? 🤔

@lavigne958
Copy link
Collaborator Author

Actually after taking a look I managed to add a test that tests the set_timeout method and measures the time too in case we don't respect the timeout value. 🥳

@lavigne958
Copy link
Collaborator Author

I was thinking, maybe it's better if I just add the missing method and nothing else. We'll see in the future to add other convenient methods ? It makes things simpler

After moving the HTTP Client to a new class, we need to add a new high
level method for users to set the HTTP timeout.

Add it to all classes which contain the HTTP client for ease of use.

Signed-off-by: Alexandre Lavigne <lavigne958@gmail.com>
@lavigne958
Copy link
Collaborator Author

due to conflicts, rebased on master branch, simplified the changes by only adding the necessary function to be backward compatible

gspread/spreadsheet.py Show resolved Hide resolved
@lavigne958 lavigne958 merged commit c5fd13e into master Mar 1, 2024
10 checks passed
@lavigne958 lavigne958 deleted the bugfix/add_set_timeout branch March 1, 2024 16:02
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.

Client has no attribute set_timeout
2 participants