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

Update StrEnum to be case insensitive #285

Merged
merged 4 commits into from Apr 12, 2023

Conversation

DarkKronicle
Copy link
Contributor

@DarkKronicle DarkKronicle commented Mar 13, 2023

Spotify recently changed their enums to be all caps in some cases. I found them specifically in album_type it's ALBUM instead of album, which breaks tekore in some cases. tekore broke because it assumed that these keys were a specific case.

The change I've implemented makes it so that when creating the tekore models, finding the AlbumType and other StrEnum objects is case insensitive. This will help future proof in case Spotify does something similar again in the future. My implementation does not modify values of the enums, just how they are stored and later indexed. If a different solution is desired, let me know.

  • Tests written with 100% coverage
  • Documentation and changelog entry written
  • All tox checks passed with an appropriately configured
    environment

@felix-hilden
Copy link
Owner

Thanks for submitting! Seems reasonable, I'd like to see some tests if you're up for it!

@DarkKronicle
Copy link
Contributor Author

Do you want me to write some new ones or run the already existing ones?

@felix-hilden
Copy link
Owner

felix-hilden commented Mar 16, 2023

Just a few new tests verifying that the new meta works 👌

@felix-hilden
Copy link
Owner

Hi! I'm fixing another issue as well and it would be good to release these at the same time. Are you down to write the tests in the near future? If not, I could just push some on this branch and we'll get it in.

@felix-hilden
Copy link
Owner

Other tests should pretty much pass now if you rebase on master!

@DarkKronicle
Copy link
Contributor Author

Ok, I'll rebase when I get some time. Feel free to yourself if that's easier.

@DarkKronicle
Copy link
Contributor Author

Wrote some tests. Realized I had accidentally set the value to lowercase in the lookup and not the key, which would have done nothing. That's fixed now.

@felix-hilden
Copy link
Owner

Thanks for testing it too! Apparently I wasn't in such a hurry :P Sorry for taking so long. It'll be released today!

@felix-hilden felix-hilden merged commit e399d34 into felix-hilden:master Apr 12, 2023
5 of 6 checks passed
felix-hilden added a commit that referenced this pull request Apr 12, 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

2 participants