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

Incorrect documentation for ImagePalette size parameter (and maybe not needed at all) #5636

Closed
rjmorris opened this issue Jul 26, 2021 · 3 comments · Fixed by #5641
Closed
Labels

Comments

@rjmorris
Copy link

The documentation for the ImagePalette initializer in version 8.3.1 says the palette parameter must be "of length size times the number of colors in mode". Therefore, for an RGB image, I would expect len(palette) == size * 3. However, the code asserts that len(palette) == size, so I believe the code and documentation are inconsistent. (The same problem existed in 8.2.0 before some ImagePalette improvements were made, so this wasn't introduced with that change.)

Furthermore, it isn't clear to me that the size parameter is needed at all. It isn't stored on self, and the only place it's used in the initializer is to assert that its value is 0 or len(palette), so it doesn't seem to provide any benefit. The only reason to keep it that I can think of is to maintain backwards compatibility with existing code that explicitly passes the parameter.

@radarhere
Copy link
Member

The incorrect documentation was added in #1381. I've created #5638 to fix it.

As background, the size parameter was originally added in #537. Before #5552, it was there to override a default length check. Afterwards though, the default length check was gone, so size is now just adding an arbitrary constraint on the user.

While I acknowledge that 8.2.0 changed ImagePalette significantly, this is removing an API parameter. If it is to be removed, deprecation is preferred first.

As an aside, you would probably appreciate knowing that there is another PR in the pipeline to update the documentation for the channel order used by the palette argument - #5599

@rjmorris
Copy link
Author

Thank you @radarhere for the background and addressing this so quickly. The changes you made in #5638 sound great to me. I came across #5599 when searching for prior discussion of this issue, so I was already familiar with that upcoming change (which is also appreciated).

@radarhere
Copy link
Member

#5638 has been merged, and I've created PR #5641 to deprecate the size parameter, resolving this.

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

Successfully merging a pull request may close this issue.

2 participants