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

added from_normalized() and get and set normalized methods for pygame.Color #2693

Merged
merged 6 commits into from
Feb 4, 2024

Conversation

XFajk
Copy link
Contributor

@XFajk XFajk commented Jan 23, 2024

this should add the feature requested in #2645
I added the from_normalized() method to pygame.Color also I added the getter and setter methods for the normalized parameter so you can change it

@XFajk XFajk requested a review from a team as a code owner January 23, 2024 19:38
Copy link
Member

@ankith26 ankith26 left a comment

Choose a reason for hiding this comment

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

For backward compatibility reasons, we cannot remove the normalize() method. However, we can definitely add a new from_normalized() class method, and a new normalized property

@XFajk
Copy link
Contributor Author

XFajk commented Jan 24, 2024

Added it back

@XFajk XFajk requested a review from ankith26 January 24, 2024 10:27
Copy link
Member

@Matiiss Matiiss left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR!

The implementation looks great!

A couple of things that need to be added:

  • docs (see the comment about the color_doc.h file)
  • tests, need to add those for the new property and classmethods, test file is test/color_test.py, just follow the existing tests for these properties and methods
  • type stubs, need to add those in buildconfig/stubs/pygame/color.pyi, again, just see how it is already done and follow that example

src_c/doc/color_doc.h Outdated Show resolved Hide resolved
src_c/color.c Outdated Show resolved Hide resolved
src_c/color.c Outdated Show resolved Hide resolved
docs/reST/ref/color.rst Outdated Show resolved Hide resolved
src_c/color.c Outdated Show resolved Hide resolved
test/color_test.py Outdated Show resolved Hide resolved
@Matiiss
Copy link
Member

Matiiss commented Jan 25, 2024

Also make sure you run python setup.py format, otherwise the linter is failing as well.

in this commit, I changed the docs a little as requested also ran the python setup.py format which has changed some files for some reason, and hopefully made all the tests for the normalized representation
@XFajk
Copy link
Contributor Author

XFajk commented Jan 26, 2024

ok I have changed the docs a bit though I am not sure if this is how you wanteded it. So check it out again and tell me if it can be that way. Hopefully, I copied/made all the tests for the normalized representation if I have missed any pls tell me I am not sure I double checked but I may have missed one or two

Copy link
Member

@Matiiss Matiiss left a comment

Choose a reason for hiding this comment

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

LGTM 👍
I assume those other random files were formatted by python setup.py format, since the changes are tiny, I assume it's fine to leave them in.

@XFajk
Copy link
Contributor Author

XFajk commented Jan 26, 2024

LGTM 👍 I assume those other random files were formatted by python setup.py format, since the changes are tiny, I assume it's fine to leave them in.

yeah exactly

@Starbuck5 Starbuck5 added New API This pull request may need extra debate as it adds a new class or function to pygame color pygame.color labels Jan 28, 2024
Copy link
Member

@MyreMylar MyreMylar left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@MyreMylar MyreMylar linked an issue Feb 4, 2024 that may be closed by this pull request
@MyreMylar MyreMylar merged commit 54a1369 into pygame-community:main Feb 4, 2024
30 checks passed
@itzpr3d4t0r itzpr3d4t0r added this to the 2.5.0 milestone Feb 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
color pygame.color New API This pull request may need extra debate as it adds a new class or function to pygame
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pygame.Color.from_normalized()
6 participants