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

Created a batch_merge function [Issue #1473] #1498

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

muddi900
Copy link
Contributor

@muddi900 muddi900 commented Aug 2, 2024

Fixes #1473

@lavigne958
Copy link
Collaborator

thanks for this contribution ! as mentioned in the associated issue the input format should be simpler. see last comment: #1473 (comment)

@alifeee
Copy link
Collaborator

alifeee commented Aug 21, 2024

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
Copy link
Collaborator

alifeee commented Aug 21, 2024

@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 ?

@muddi900
Copy link
Contributor Author

muddi900 commented Sep 2, 2024

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.

@alifeee
Copy link
Collaborator

alifeee commented Sep 10, 2024

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.

@muddi900
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request. Batch Merge in worksheet class
3 participants