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 combine_merged_cells when using from a range that doesn't start at A1 #1335

Conversation

alifeee
Copy link
Collaborator

@alifeee alifeee commented Oct 25, 2023

closes #1330

add start_row_index and start_col_index to utils.combined_merge_values so that ranges that don't start at col 0, row 0 (i.e., A1) can have their merged cells merged.

n.b., not starting from the top left introduces more issues with combine_merged_cells -> #1334

This code is added to worksheet.get_values. This is to get the start_row_index and start_col_index from the requested range.

grid_range = a1_range_to_grid_range(
get_a1_from_absolute_range(range_name),
)
return combined_merge_values(
worksheet_metadata=worksheet_meta,
values=vals,
start_row_index=grid_range.get("startRowIndex", 0),
start_col_index=grid_range.get("startColumnIndex", 0),
)

  • This may fail if a named range is used (see A1 notation docs) - add tests? or do not allow combine_merged_cells if a named range is used? since we don't know the start index?

@alifeee alifeee added the Bug label Oct 25, 2023
@alifeee alifeee added this to the 5.12.1 milestone Oct 25, 2023
@alifeee alifeee self-assigned this Oct 25, 2023
@alifeee alifeee marked this pull request as ready for review October 25, 2023 12:50
@alifeee alifeee changed the title 1330 combining merged cells producing incorrect result when used on a non default cell range fix combine_merged_cells when using from a range that doesn't start at A1 Oct 25, 2023
@alifeee alifeee requested review from lavigne958 and removed request for lavigne958 November 6, 2023 22:40
@alifeee
Copy link
Collaborator Author

alifeee commented Nov 14, 2023

@lavigne958 once this is approved and merged, we can release 5.12.1.

This release should be the last before 6.0.0, so without another (5.12.2 e.g.), this will also the last chance to get an updated README onto the PyPi (the readme on the GitHub will always stay up to date)

@lavigne958
Copy link
Collaborator

Sure I need to take a look properly when I have time. This week should do.

@lavigne958
Copy link
Collaborator

  • This may fail if a named range is used (see A1 notation docs) - add tests? or do not allow combine_merged_cells if a named range is used? since we don't know the start index?

When we request: same value for merged cells, we make a second call to the spreadsheet right ? to pull all the merges.
Can we extract the coordinates of the named range at the same time ? 🤔

gspread/utils.py Outdated Show resolved Hide resolved
gspread/worksheet.py Outdated Show resolved Hide resolved
@alifeee
Copy link
Collaborator Author

alifeee commented Nov 20, 2023

When we request: same value for merged cells, we make a second call to the spreadsheet right ? to pull all the merges.
Can we extract the coordinates of the named range at the same time ? 🤔

Yes. In fact, we already request the spreadsheet metadata to get the merges:

spreadsheet_meta = self.spreadsheet.fetch_sheet_metadata()

So, I have reused this metadata to also find the grid_range of a named range:

# deal with named ranges
named_ranges = spreadsheet_meta.get("namedRanges", [])
# if there is a named range with the name range_name
if any(
range_name == ss_namedRange["name"]
for ss_namedRange in named_ranges
if ss_namedRange.get("name")
):
ss_named_range = finditem(
lambda x: x["name"] == range_name, named_ranges
)
grid_range = ss_named_range.get("range", {})

and other range types are in a different branch

# norrmal range_name, i.e., A1:B2
elif range_name is not None:
a1 = get_a1_from_absolute_range(range_name)
grid_range = a1_range_to_grid_range(a1)
# no range_name, i.e., all values
else:
grid_range = worksheet_meta.get("basicFilter", {}).get("range", {})

This may be too much logic for get_values and you might prefer it be offloaded to a utils function. It also is only implemented for get_values, as get and get_values have not yet been combined (happens in v6.0.0)

@lavigne958
Copy link
Collaborator

let's get release 5.12.1 out

@alifeee
Copy link
Collaborator Author

alifeee commented Nov 28, 2023

agreed. Does that include a fix for #1354?

@lavigne958
Copy link
Collaborator

I dont't think we should include it, it still needs a bit more thought about the resolution of the problem.
This is release has already quiet a bit of changes to provide.
In my opinion: we should merge this one and release.

#1354 can be addressed right after.

@alifeee alifeee merged commit c1f324c into master Nov 29, 2023
12 checks passed
@alifeee alifeee deleted the 1330-combining-merged-cells-producing-incorrect-result-when-used-on-a-non-default-cell-range branch November 29, 2023 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Combining merged cells producing incorrect result when used on a non-default cell range
2 participants