Open muddi900 opened 2 months ago
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
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 ?
@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 ?
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.
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.
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.
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 ?
Fixes #1473