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

combine_merged_cells cannot capture all merges #1334

Open
alifeee opened this issue Oct 25, 2023 · 4 comments
Open

combine_merged_cells cannot capture all merges #1334

alifeee opened this issue Oct 25, 2023 · 4 comments
Labels
Need investigation This issue needs to be tested or investigated

Comments

@alifeee
Copy link
Collaborator

alifeee commented Oct 25, 2023

Merges are actually just one value in the top left cell, and some metadata. If the top-left cell is not in your requested range, you cannot faithfully recreate the merge. For example:

image

If you called worksheet.get_values("F3:H4", combine_merged_cells=True), what would you expect?

1

"big merge" 2 3
4 5 6

or 2

2 3
4 5 6

We cannot recreate 1 without added complexity. We must return 2. This may be unexpected.

combine_merged_cells was added in #1215. Its addition has brought several problems (#1298, #1330).

What should we do? If we leave it in, it will always have this issue. If we remove it, it will be sad.

@alifeee alifeee added the Need investigation This issue needs to be tested or investigated label Oct 25, 2023
@alifeee alifeee added this to the 6.0.0 milestone Oct 25, 2023
@lavigne958
Copy link
Collaborator

Users will expect the value Big Merge in F3. We should add the extra complexity. When we list the merges and see that one overlaps the requested range then we should get and fill what is necessary.

For sure this will take some time and either add a very large util function or introduce a big code change in Worksheet.py

We should start working on it after 6.0.0 is out that would be a great improvement to bring after the release.

@lavigne958
Copy link
Collaborator

Thinking about it I would solve it as:

  • we get values from a range
  • if user requests combined merged values
  • get the list of merges from the API
  • check if any merge range overlaps the requested range in all boundaries
    • in order to solve this, we might find some already optimized algorithm online 🤔
    • we could rely on some third party library too (check some classic packages like bumpy etc )
  • for any overlapping merge set the values in the "empty" cells.

A different solution that we can try is: open an issue on Google side and request some API changes or some extra parameters that does it all for us.

@alifeee
Copy link
Collaborator Author

alifeee commented Oct 26, 2023

I think this is a very large change with lots to think about. combine_merged_cells has already caused two minor releases due to bugs (#1298 #1330). I am worried that making it even more complex is the wrong direction to go in. I think it is a small feature in the context of the size of gspread and not worth the effort to fix this issue.

I would rather add a warning that says it will not pull in data from outside the requested range, or as you say submit a request to Google's API.

@alifeee
Copy link
Collaborator Author

alifeee commented Oct 28, 2023

pushing this past 6.0.0 release

@alifeee alifeee removed this from the 6.0.0 milestone Oct 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Need investigation This issue needs to be tested or investigated
Projects
None yet
Development

No branches or pull requests

2 participants