Skip to content

Add Color.__bytes__ #3547

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Matiiss
Copy link
Member

@Matiiss Matiiss commented Aug 3, 2025

Fixes #3546

@Matiiss Matiiss requested a review from a team as a code owner August 3, 2025 20:50
Copy link
Member

@oddbookworm oddbookworm left a comment

Choose a reason for hiding this comment

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

Where docs?

@Starbuck5
Copy link
Member

I was thinking docs aren't necessary for this change, since it makes color behave like a normal sequence would.

However I do think it would be a good idea to put a comment in the code explaining why it's necessary to have the method explicitly written out.

@oddbookworm
Copy link
Member

I was thinking docs aren't necessary for this change, since it makes color behave like a normal sequence would.

I disagree. A versionadded note at least, that way if someone does use this functionality, they at least have some way to reference that it's broken pre-2.5.whatever

@oddbookworm
Copy link
Member

I was thinking docs aren't necessary for this change, since it makes color behave like a normal sequence would.

I disagree. A versionadded note at least, that way if someone does use this functionality, they at least have some way to reference that it's broken pre-2.5.whatever

A user-side implementation could look like:

if pygame.version.vernum < (2, 5, 6):
    color_bytes = bytes([color.r], [color.g], [color.b], [color.a])
else:
    color_bytes = bytes(color)

@aatle
Copy link
Contributor

aatle commented Aug 4, 2025

I was thinking docs aren't necessary for this change, since it makes color behave like a normal sequence would.

I disagree. A versionadded note at least, that way if someone does use this functionality, they at least have some way to reference that it's broken pre-2.5.whatever

A user-side implementation could look like:

if pygame.version.vernum < (2, 5, 6):
    color_bytes = bytes([color.r], [color.g], [color.b], [color.a])
else:
    color_bytes = bytes(color)

Yeah, a versionadded at least.

Also that snippet doesn't work, here are two ways to convert colors currently:

bytes(list(color))
int(color).to_bytes(4)  # over 2x faster

@aatle aatle added the color pygame.color label Aug 4, 2025
@ankith26
Copy link
Member

ankith26 commented Aug 4, 2025

Also I'm +1 for andrews suggestion for adding this in a versionadded note. People need to know to not use this if they are targetting older pygame-ce versions

Matiiss and others added 3 commits August 7, 2025 02:15
Co-authored-by: Ankith <itsankith26@gmail.com>
…hods (as opposed to through a special struct slot)
@Matiiss Matiiss requested review from oddbookworm and ankith26 August 6, 2025 23:54
Copy link
Member

@oddbookworm oddbookworm left a comment

Choose a reason for hiding this comment

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

I have a couple of comments, but I'm not so set on these that I can't be convinced otherwise if a good argument gets thrown at me

Comment on lines +263 to +269
/**
* While object.__bytes__(self) is listed in the Data Model reference (see:
* https://docs.python.org/3/reference/datamodel.html#object.__bytes__) it
* does not appear to have a PyTypeObject struct analog (see:
* https://docs.python.org/3/c-api/typeobj.html), so we declare it for the
* type as any other custom method.
*/
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this giant comment is strictly necessary in the middle of this large array definition

@@ -92,6 +92,11 @@
:returns: a newly created :class:`Color` object
:rtype: Color

.. versionchanged:: 2.5.6
Copy link
Member

Choose a reason for hiding this comment

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

I still think versionadded is more appropriate because versionchanged indicates that we changed behavior on our end, instead of overriding default python behavior by adding a new method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
color pygame.color
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Color.__bytes__
5 participants