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

get_all_worksheet_values feature #1180

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

swimninja247
Copy link

Addresses #1002.

The function returns a dict of all the worksheet values, mapped by the worksheet name/title.
I have not written tests for this function yet.

@lavigne958
Copy link
Collaborator

Hi thank you for your contribution, I'll have a proper look at the code when I have some free time (very soon).

I looked at it quickly and I see some changes to be done. I'll come back with a complete review.

gspread/spreadsheet.py Outdated Show resolved Hide resolved
gspread/spreadsheet.py Outdated Show resolved Hide resolved
gspread/spreadsheet.py Outdated Show resolved Hide resolved
gspread/utils.py Outdated Show resolved Hide resolved
@swimninja247
Copy link
Author

Thanks for the review. I resolved your comments and fixed the docstrings for the code I added. Cheers

Copy link
Collaborator

@lavigne958 lavigne958 left a comment

Choose a reason for hiding this comment

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

took me some time but I finally found the time to review your PR.

They are some small adjustments that need to be addressed.

Comment on lines +752 to +754
if skip_worksheet_titles is None:
skip_worksheet_titles = []

Copy link
Collaborator

Choose a reason for hiding this comment

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

if you set it to [] empty list when it's not set, then just set the default value to [] in the argument in the method definition.

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose the initial solution was right, because usage of a mutable object as default argument can follow wrong behaviour. You can see example of such problem there


ranges = []

for worksheet in self.worksheets().worksheets():
Copy link
Collaborator

Choose a reason for hiding this comment

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

that won't work, it should be only self.worksheets()


for worksheet in self.worksheets().worksheets():
if worksheet.title not in skip_worksheet_titles:
ranges.append(worksheet.title)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here we should protect the worksheet title using the util function absolute_range_name

@@ -27,6 +27,8 @@
URL_KEY_V1_RE = re.compile(r"key=([^&#]+)")
URL_KEY_V2_RE = re.compile(r"/spreadsheets/d/([a-zA-Z0-9-_]+)")

TITLE_RANGE_RE = re.compile(r"'(.*?)'!.*")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This won't always work, here we can have 2 cases:

  1. names with a blank space or that starts with ' so they start with a ' but they may have multiple ' surrounding the actual name
  2. no blank space, no ' at starts or end. so it's a single string with only characters.

The regex should be improved to match all possible titles.

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.

3 participants