get_all_worksheet_values feature#1180
Conversation
|
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. |
|
Thanks for the review. I resolved your comments and fixed the docstrings for the code I added. Cheers |
lavigne958
left a comment
lavigne958
left a comment
There was a problem hiding this comment.
took me some time but I finally found the time to review your PR.
They are some small adjustments that need to be addressed.
| if skip_worksheet_titles is None: | ||
| skip_worksheet_titles = [] | ||
|
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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(): |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
Here we should protect the worksheet title using the util function absolute_range_name
| 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"'(.*?)'!.*") |
There was a problem hiding this comment.
This won't always work, here we can have 2 cases:
- names with a
'so they start with a'but they may have multiple'surrounding the actual name - no
'at starts or end. so it's a single string with only characters.
The regex should be improved to match all possible titles.
3 participants
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.