-
-
Notifications
You must be signed in to change notification settings - Fork 176
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
Add Color.hex
#3379
base: main
Are you sure you want to change the base?
Add Color.hex
#3379
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks! 💯
As it is new API, requesting atleast 2 more approvals in addition to mine, having more eyes on this PR will be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, left a couple comments, but nothing that would necessarily have to block this PR
docs/reST/ref/color.rst
Outdated
| :sl:`Gets or sets the stringified hexadecimal representation of the Color.` | ||
| :sg:`hex -> str` | ||
|
||
The strigified hexadecimal representation of the Color. The hexadecimal string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strigified -> stringified
docs/reST/ref/color.rst
Outdated
| :sg:`hex -> str` | ||
|
||
The strigified hexadecimal representation of the Color. The hexadecimal string | ||
is formatted as ``"#rrggbbaa"`` where rr, gg, bb, and aa are 2 digits hex numbers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
digits -> digit
optional: 2 -> two
all suggestions are good I'll fix them today |
The latest commit broke formatting of a test file. |
This little maneuver took 15 hours! on my Android
Checks seem to be failing because of a bug in package |
I don't really see this why should be added. The FAQ in the PR description seems to assume this is a thing people want to do, but I can't think of why I would want to generate a hex string from a color object. |
The primary reason is for displaying and inputting for user, not really for internal code. The hex rgb representation is the standard way to display, port, input colors; every color picker supports this format. Therefore it makes sense to add a shortcut. If a pygame program wanted to allow the user to pick any color for something, this would be useful for both labelling and inputting the color. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to add a corresponding classmethod from_hex()
, in accordance with every other settable color format. So that it's possible to use the setter in an expression.
Not necessary, it's already possible like so pygame.Color("#hexstr") |
I still think it could be beneficial to add it. It is explicit and could differentiate hex strings from color name strings (useful for input), plus more consistency. |
Followed the New API routine for this property. FAQ below.
Why, if it can be done in one line?
That's not a reason not to add a shortcut. Other color shortcuts like Color.normalized can be achieved in one line aswell, plus, how to get the Color hex with one line is not very straight forward.
Can't you use the built in
hex
function?That function wasn't made for correct representation of hex colors, so
Color(hex(Color))
can easily error, for example in the following cases:hex(Color(1, 2, 3)) -> "0x10203ff"
(no trailing zero; ValueError) orhex(Color(0, 0, 0, 255)) -> "'0xff"
(not long enough; ValueError).