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

Adds support for the "other" category In sprites #52

Merged
merged 7 commits into from
Apr 6, 2023

Conversation

Deleca7755
Copy link
Contributor

Sorry If It succ I never make significant pull requests or... program at all often ( only recently started a meaningful project which I'm using your library for ), but It worked for me

Copy link
Owner

@beastmatser beastmatser left a comment

Choose a reason for hiding this comment

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

First of all thank you for contributing, I just have a few remarks about the code you wrote. I would like to keep the original structure of the data that is collected, meaning that the additions you made should be accesed by pokemon.sprites.other.dream_world for example. So, Other should get its own class and that is where the three attributes should be put. Also notice that the class DreamWorld and OfficialArtwork are identical, therefore I believe it would be best to make a parent class and then define both classes by just inheriting from this newly made class. Lastly, you correctly check if the other attribute could be None by using an if-structure. Instead, you could use a one-liner for this, this is a purely asthetic chance made for consistency. Thank you!

beastmatser

This comment was marked as resolved.

@beastmatser beastmatser dismissed their stale review April 6, 2023 14:48

Code was already adequate

@beastmatser beastmatser merged commit 26081ae into beastmatser:main Apr 6, 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.

2 participants