gspread icon indicating copy to clipboard operation
gspread copied to clipboard

Created a `batch_merge` function [Issue #1473]

Open muddi900 opened this issue 1 year ago • 3 comments

Fixes #1473

muddi900 avatar Aug 02 '24 06:08 muddi900

thanks for this contribution ! as mentioned in the associated issue the input format should be simpler. see last comment: https://github.com/burnash/gspread/issues/1473#issuecomment-2295227936

lavigne958 avatar Aug 18 '24 11:08 lavigne958

looks wonderful ;)

~~this should have a test~~ I have bad eyes. It does have a test.

@muddi900 would you like any help with the cassettes ?

alifeee avatar Aug 21 '24 13:08 alifeee

@muddi900 I have updated your test and cassette

the function is called like

       sheet.batch_merge(
            {
                "A1:B2": utils.MergeType.merge_all,
                "C2:D2": utils.MergeType.merge_all,
                "C3:C4": utils.MergeType.merge_all,
            }
        )

The normal merge function (I think) can be called like (i.e., without merge type, uses default merge_all)

sheet.merge("A1:B2")

It would be nice if batch_merge could be called without specifying the merge type for each range, as I imagine most people will want to use merge_all and not merge_column or merge_row, so it would be nice to just use the default, something like

sheet.batch_merge(
  [
    "A1:B2",
    "C2:D2",
    "C3:C4"
  ]
)

I think this is what @muddi900 and @lavigne958 were discussing in #1473 with regard to the type of the argument

What do you both think ?

alifeee avatar Aug 21 '24 14:08 alifeee

I, as a user, would prefer my implementation, as that would not require a merge type declaration for each merge. And it would allow adding a merge type for a specific cases.

muddi900 avatar Sep 02 '24 13:09 muddi900

I agree @muddi900. What do you think @lavigne958?

I see three options

1

sheet.batch_merge(
  [
    "A1:B2",
    "C2:D2",
    "C3:C4"
  ]
)

2

sheet.batch_merge(
  [
    {"range": "A1:B2"},
    {"range": "C2:D2"},
    {"range": "C3:C4", "merge_type": utils.MergeType.merge_all}
  ]
)

3

sheet.batch_merge(
            {
                "A1:B2": utils.MergeType.merge_all,
                "C2:D2": utils.MergeType.merge_all,
                "C3:C4": utils.MergeType.merge_all,
            }
        )

Under the assumption that most of the time, a user will not care to specify the merge type...

I think 1 is the most user friendly option, but I'm not sure how to specify merge types (a second array, and zip them? I don't like it so much...)

I think 2 works, but is still quite complicated.

I think 3 is the most complicated as it does not allow missing out merge type.

I suggest some experimental options

4

sheet.batch_merge(
  [
    "A1:B2",
    ("C2:D2", utils.MergeType.merge_all),
    "C3:C4"
  ]
)

5

sheet.batch_merge(
  [
    "A1:B2",
     {"range": "C3:C4", "merge_type": utils.MergeType.merge_all},
    "C3:C4"
  ]
)

I think my overall preference is 4, as it does not require knowledge of what strings to use (i.e., "range" and "merge_type") nor to specify merge_type for every range.

alifeee avatar Sep 10 '24 18:09 alifeee

It would be better if the list has a unified type.

the reason I opted for dict instead of tup because the latter may lead to user errors.

muddi900 avatar Sep 11 '24 05:09 muddi900

Hi I agree with @muddi900 , mixed inputs that can be string or tuple(string, MergeType) is confusing.

I still think option 3 is the most explicit one with the least necessary inputs to pass (a string for the key and a MergeType for the value).

I understand that option 2 could be simpler for the users, it allows them to reduce code and it's very easy for us to set the default value when reading the dictionary.

Option 1 to me does not provide enough flexibility for the user, if any user needs to do some column or row merge then the feature is useless for that user.

I would accept option 2 if that suits everyone ?

lavigne958 avatar Sep 22 '24 22:09 lavigne958