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] run_batch with requests from multiple spreadsheets #582

Open
wants to merge 2 commits into
base: staging
Choose a base branch
from

Conversation

Sevans711
Copy link

Previously, run_batch behavior was not correct (or at least, not intuitive - there was a return statement direcly inside a for loop).
Previous behavior:

  1. batch update all requests for the first spreadsheet in batched_requests
  2. return reply from that update, without clearing self.batched_requests

New behavior:

  1. batch update all requests for each spreadsheet in batched_requests (one spreadsheet at a time)
  2. set self.batched_requests = dict()
  3. return result = list of replies, with length equal to number of spreadsheets. For backwards compatibility, when there is only one spreadsheet the default behavior is to return result[0] instead.

pygsheets/sheet.py Outdated Show resolved Hide resolved
@nithinmurali
Copy link
Owner

Thank you for the PR! please add a test to verify if its all good.

@Sevans711
Copy link
Author

Okay, I did this renaming: _return_list_if_single --> return_list_if_single.

Can you help me with "add a test to verify if it's all good"? I'm relatively new at contributing to other repositories. Also, thank you for building/maintaining pygsheets!

@nithinmurali
Copy link
Owner

So we need to add some tests to verify that the functionality that you added works as intended. For pygsheets we only have tests that use the real API (end to end tests). You can find them here.

For this PR you can write a test that makes some batch requests in multiple sheets that would have failed with the old code but with this fix its fixed.

Please refer to existing examples on how to write tests (you can also find examples in old PRs #571 )

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