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

Revert to using enums for relevant classses (Quality etc) #243

Closed
exislow opened this issue Mar 20, 2024 · 9 comments
Closed

Revert to using enums for relevant classses (Quality etc) #243

exislow opened this issue Mar 20, 2024 · 9 comments

Comments

@exislow
Copy link
Contributor

exislow commented Mar 20, 2024

Thanks for the new release v0.7.5. I was wondering what, the reason is for the enum cleanups, starting here? https://github.com/tamland/python-tidal/blame/v0.7.5/tidalapi/media.py#L56

Appreciate any information.

@tehkillerbee
Copy link
Collaborator

Main reason was to avoid needing to call .value to access Enum members, as we usually use their string value anyways.

@exislow
Copy link
Contributor Author

exislow commented Mar 22, 2024

This is is really too bad! Since this project is mainly used as a library by other projects, it makes it really hard to iterate through these "constants". For example: https://github.com/exislow/tidal-dl-ng/blob/e1dfc9ae0b86601e950387882533ab7ea6684f22/tidal_dl_ng/gui.py#L175

I cannot do this anymore to simply populate the GUI with the available quality settings. And I don't see how I can solve this easily not re-assigning all former Enum values.

Would it be possible, if you at least use dataclasses (https://docs.python.org/3/library/dataclasses.html)? Actually Enums would be best in my opinion, also because this would be semantically correct.

@tehkillerbee
Copy link
Collaborator

IMO, it shouldn't be too hard to iterate over the Quality member variables but I see it adds extra work for the libraries that requires this. Besides, the member variables are not immutable, which is probably not the best idea.

Since we do not have StrEnum class available unless Python 3.11, an option could be to switch back to the Enum and add

class Quality(str, Enum):

    LOW = 'LOW'
    HIGH = 'HIGH'

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

Then we avoid calling .value whenever the string value is needed, which is usually the case internally in tidalapi.

@tehkillerbee tehkillerbee reopened this Mar 22, 2024
@tehkillerbee tehkillerbee changed the title Why did you do an "enum cleanup" Revert to using enums for Quality classes Mar 22, 2024
@exislow
Copy link
Contributor Author

exislow commented Mar 22, 2024

I actually see no pain in using .value on an Enum. It's very straight forward and just a few characters more. But you have maybe some other reasons, which really annoys you in this case. Do you plan to raise the supported Python version to 3.11? Otherwise your proposed solution inheriting from str, Enum would be the best compromise, I guess.

@tehkillerbee tehkillerbee changed the title Revert to using enums for Quality classes Revert to using enums for relevant classses (Quality etc) Mar 22, 2024
@exislow
Copy link
Contributor Author

exislow commented Apr 1, 2024

Do you like me to get you an PR on this for the upcoming release?

@tehkillerbee
Copy link
Collaborator

tehkillerbee commented Apr 1, 2024

I haven't had much time due to other commitments, sorry, so I have not had time to look at this yet.

It would be great if you have the time, thanks.

@exislow exislow mentioned this issue Apr 2, 2024
@exislow
Copy link
Contributor Author

exislow commented Apr 2, 2024

I got you a PR: #248

@exislow
Copy link
Contributor Author

exislow commented Apr 11, 2024

Thanks for mergin! Have you already a date for the next tidalapi release in mind?

@tehkillerbee
Copy link
Collaborator

@exislow I will try to see if I can get next release ready by the end of the week, at least with the changes from the latest few PRs.

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

No branches or pull requests

2 participants