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

Remplace named tuples with enums #1250

Merged

Conversation

lavigne958
Copy link
Collaborator

Most of the parameters for the API are ENUMs

Replace the named tuples that are not fitted for that with enums.

Each enum hold one of the choices.
The actual string value must be extracted in order to pass strings only to the API using the function extract_enum_value using the enum.value field if the enum is not None.

this forces us to filter all arguments that are passed when they can possibly be None.

thought about setting the default value for each instead but this changes the API call made to the API and that could lead to unwanted behavior for the users.

Possible way was to add a new value to each enum: default = None which will result in a None value so it won't be part of the API call, but this requires all method to have that value as default value, and if a user (for some reason) sets one of the parameter to param=None then the code breaks and requires that user to change to param=MyEnum.none which is not user friendly. To me any function with default empty param should be either None or some default container like param=[] etc.

so far this is the less intrusive for users and keeps the API calls as is and use the default behavior from google.

closes #1222

Most of the parameters for the API are `ENUM`s

Replace the named tuples that are not fitted for that
with enums.

Each enum hold one of the choices.
The actual string value must be extracted in order
to pass strings only to the API using the function `extract_enum_value`
using the `enum.value` field if the enum is not `None`.

this forces us to filter all arguments that are passed when they
can possibly be `None`.

thouhgt about setting the default value for each instead
but this changes the API call made to the API and that could lead
to unwanted behavior for the users.

closes #1222
@lavigne958 lavigne958 requested a review from alifeee July 5, 2023 15:31
@lavigne958 lavigne958 self-assigned this Jul 5, 2023
gspread/worksheet.py Outdated Show resolved Hide resolved
@alifeee
Copy link
Collaborator

alifeee commented Jul 6, 2023

I cannot find the comment we discussed it originally, but why not use strEnums, so that you do not have to use .value?

@lavigne958
Copy link
Collaborator Author

I cannot find the comment we discussed it originally, but why not use strEnums, so that you do not have to use .value?

We can't use StrEnum they were introduced in python3.11 only :-( I though about it, it would make life much easier !

@alifeee
Copy link
Collaborator

alifeee commented Jul 6, 2023

We can't use StrEnum they were introduced in python3.11 only :-( I though about it, it would make life much easier !

What about with StrEnum: https://pypi.org/project/StrEnum/

It supports Python 3.7+

@lavigne958
Copy link
Collaborator Author

We can't use StrEnum they were introduced in python3.11 only :-( I though about it, it would make life much easier !

What about with StrEnum: https://pypi.org/project/StrEnum/

It supports Python 3.7+

wow awesome ! 🤩 I'll update the code to use it then it makes like so much easier (If everything goes according to plan lol)

@lavigne958 lavigne958 requested a review from alifeee July 6, 2023 15:42
gspread/worksheet.py Outdated Show resolved Hide resolved
gspread/worksheet.py Outdated Show resolved Hide resolved
@lavigne958
Copy link
Collaborator Author

you already approved but as I made changes I wait for your review before merging.

Copy link
Collaborator

@alifeee alifeee left a comment

Choose a reason for hiding this comment

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

lgtm!

it's nice that worksheet only has docs changes now that the enums can be used where strings are used.

question: why all the whitespace change?

image

ready to merge! good work! ❤

@lavigne958
Copy link
Collaborator Author

lavigne958 commented Jul 9, 2023

lgtm!

it's nice that worksheet only has docs changes now that the enums can be used where strings are used.

Yes that's right, so much easier.

question: why all the whitespace change?

Because the RST documentation format works with indentation. The length (number of whitespaces) need to create a block, a list, a warning etc is aligned with the parent block. All of those where not aligned so I aligned them to properly render the documentation.

image

ready to merge! good work! ❤

Thanks, and thanks to you for finding the str enum class

@lavigne958 lavigne958 merged commit 36b796e into feature/release_6_0_0 Jul 9, 2023
10 checks passed
@alifeee alifeee deleted the feature/replace_named_tuple_to_enum branch July 9, 2023 21:21
@alifeee alifeee mentioned this pull request Oct 31, 2023
@alifeee alifeee mentioned this pull request Jan 26, 2024
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

2 participants