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

Remove StrEnum dependency #1462

Closed
alifeee opened this issue May 1, 2024 · 6 comments · Fixed by #1469
Closed

Remove StrEnum dependency #1462

alifeee opened this issue May 1, 2024 · 6 comments · Fixed by #1469
Labels
dependencies Pull requests that update a dependency file
Milestone

Comments

@alifeee
Copy link
Collaborator

alifeee commented May 1, 2024

gspread dependencies are:

gspread/pyproject.toml

Lines 30 to 34 in c740b88

dependencies = [
"google-auth>=1.12.0",
"google-auth-oauthlib>=0.4.1",
"StrEnum==0.4.15",
]

StrEnum amounts to

class StrEnum(str, enum.Enum):
    def __new__(cls, value, *args, **kwargs):
        if not isinstance(value, (str, enum.auto)):
            raise TypeError(
                f"Values of StrEnums must be strings: {value!r} is a {type(value)}"
            )
        return super().__new__(cls, value, *args, **kwargs)

    def __str__(self):
        return str(self.value)

    def _generate_next_value_(name, *_):
        return name

We ought not need to rely on a dependency, and can think of a way of including this code in the project, so that the dependencies are just google auth libraries.

@lavigne958 thoughts?

@alifeee alifeee added the dependencies Pull requests that update a dependency file label May 1, 2024
@lavigne958
Copy link
Collaborator

Hi that's true, it's simple enough to be vendored (meaning we copy/paste the current code in our repository)

the pros & cons:

pros:

  • less dependencies
  • users don't need to rely on this dependency too

cons:

  • in case of any changes in python we need to fix it ourselves
  • if some improvements are added we need to add them ourselves.

I have no strong preference 🙄

do we have a major/manior reason to do this move ? is this blocking some users or something ?

@alifeee
Copy link
Collaborator Author

alifeee commented May 10, 2024

I think this is something that will not change very often. We basically just use it to polyfill old Python versions, and do not use any advanced features of StrEnum

gspread is usable before without installing StrEnum and I think this is nicer.

@alifeee alifeee added this to the 6.2.0 milestone May 10, 2024
@muddi900
Copy link
Contributor

StrEnum is part of 3.11+.

Wouldn't this need to be conditional then?

@alifeee
Copy link
Collaborator Author

alifeee commented May 10, 2024

Context:

#1250 (comment)

#1340

I think there are many "solutions" to this issue, and they all depend on what "problem" we are trying to solve.

Personally, I think the dependency is not needed, especially as we have so few dependencies to begin with (apart from the required Google Auth, we have none (or, only StrEnum, which I argue is unneeded))

@muddi900
Copy link
Contributor

Yeah I just checked the StrEnum implementation in cpyton, it's not more efficient to begin with.

I can add it, if y'all would like.

@alifeee
Copy link
Collaborator Author

alifeee commented May 10, 2024

I can add it, if y'all would like.

That would be grand 😊

I think there are many ways to add this. One would be to provide this StrEnum class/subclass in the utils.py file. Of course, the tests would be nice to port over too.

Would you like any guidance? I trust you can make a good change :)

muddi900 added a commit to muddi900/gspread that referenced this issue May 15, 2024
@lavigne958 lavigne958 modified the milestones: 6.2.0, 6.1.1 May 15, 2024
lavigne958 added a commit that referenced this issue May 15, 2024
Remove StrEnum dependency and added custom class[issue #1462]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants