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

Fixed default value of merge_type parameter in merge_cells function docstring. #1373

Merged
merged 3 commits into from Dec 31, 2023
Merged

Conversation

neolooong
Copy link
Contributor

Fixed default value of merge_type parameter in merge_cells function docstring.

@alifeee
Copy link
Collaborator

alifeee commented Dec 27, 2023

thanks, this is helpful! I always love a docs change :)

got me thinking, why aren't we using a tuple for this parameter? like for DateTimeOption?

gspread/gspread/utils.py

Lines 41 to 43 in c3b61d0

DateTimeOption = namedtuple(
"DateTimeOption", ["serial_number", "formatted_string", "formated_string"]
)("SERIAL_NUMBER", "FORMATTED_STRING", "FORMATTED_STRING")

perhaps we could change it... @lavigne958 thoughts ?

@lavigne958
Copy link
Collaborator

Thanks for the doc update.

I think we missed it 😮
Yep we can upgrade it to a namedtuple but we should make sure people can keep using strings and it works. That I'm not sure 😬 if that does not work we can't change it and should just update the documentation

@neolooong
Copy link
Contributor Author

I am sure that after changing to namedtuple, people still can use string.

@alifeee
Copy link
Collaborator

alifeee commented Dec 28, 2023

quick change! it looks good :)

now is probably also the time to add a test for merge_cells (and a merge_type test too), since there is not one in worksheet_test.py

you are welcome to give this a go, or one of us will do it if you would rather not deal with the tests :)

Copy link
Collaborator

@lavigne958 lavigne958 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the fix !
I agree if you feel like it, feel free to add a test on merge cells, if not, no worries we'll add them.

Tested it, it works in all bellow cases:

  • with sheet.merge_cell("A1:B2", "MERGE_ALL")
  • with sheet.merge_cell("A1:B2", gspread.utils.MergeType.merge_all)
    which makes it ISO functional with current code base 👍

@alifeee
Copy link
Collaborator

alifeee commented Dec 29, 2023

super. we will merge this and add a test for merge_cells in a week or so if there is no more activity here :)

Copy link
Collaborator

@lavigne958 lavigne958 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the new tests !

@lavigne958 lavigne958 merged commit b947b21 into burnash:master Dec 31, 2023
6 checks passed
@lavigne958 lavigne958 added this to the 5.12.4 milestone Dec 31, 2023
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.

None yet

3 participants