-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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 enums, deprecate constants #5954
Conversation
This is a good way to group things together, but for backwards compatibility we have each one doubled up: Image.ROTATE_90
Image.Transpose.ROTATE_90
Image.ROTATE_180
Image.Transpose.ROTATE_180
Image.ROTATE_270
Image.Transpose.ROTATE_270
# etc. Although Is the idea be that end user code should be updated to use the enum form? Do we keep the old constants "forever"? Or do we want to deprecate the old constants and remove them in Pillow 10 (15 months' deprecation period)? |
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.
To me, this change looks great! I'm a guest in this codebase, so can't judge it completely - thats the only reason I'm commenting and not approving.
One small remark would be that some of the introduced enum classes have names that are a bit generic (like Encoding) and might lead to naming collisions later.
BLP_ENCODING_UNCOMPRESSED = 1 | ||
BLP_ENCODING_DXT = 2 | ||
BLP_ENCODING_UNCOMPRESSED_RAW_BGRA = 3 | ||
class Format(IntEnum): |
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.
To avoid potential naming collisions, what about BlpFormat?
BLP_ALPHA_ENCODING_DXT3 = 1 | ||
BLP_ALPHA_ENCODING_DXT5 = 7 | ||
|
||
class Encoding(IntEnum): |
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.
To avoid potential naming collisions, what about BlpEncoding?
UNCOMPRESSED_RAW_BGRA = 3 | ||
|
||
|
||
class AlphaEncoding(IntEnum): |
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.
For consistency, what about BlpAlphaEncoding?
When you talk about naming collisions, I find it hard to imagine that we'd want to add another module-level variable called I was trying to avoid users writing >>> from PIL import BlpImagePlugin
>>> BlpImagePlugin.BlpFormat.JPEG Instead, >>> from PIL import BlpImagePlugin
>>> BlpImagePlugin.Format.JPEG seems cleaner to me. |
c0b5a26
to
f9c928c
Compare
Ok, I've updated the tests, documentation, and deprecated the original constants. I've also deprecated the "backwards compatibility" constants for Resampling, and deprecated |
Let's also add this to the "Deprecations and removals" page and release notes. |
Thanks! |
c.f. python-pillow/Pillow#5954 also (preumptively) support Pillow 10
https://pillow.readthedocs.io/en/stable/releasenotes/9.1.0.html#constants python-pillow/Pillow#5954 Signed-off-by: Anders Kaseorg <andersk@mit.edu>
Resolves #5875, by replacing various sets of constants with
IntEnum
classes. This would seem to be the exact scenario for which enums were added to Python.I have retained backwards compatibility by including lines like
globals().update(enum.__members__)
.I have updated internal usage to use the enums, but not the tests or documentation.