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

Improved handling of PPM header #5121

Merged
merged 23 commits into from Mar 4, 2022
Merged

Conversation

Piolie
Copy link
Contributor

@Piolie Piolie commented Dec 22, 2020

Addresses #5104. That discussion didn't get any replies, so I decided to open this PR to make my intentions more explicit.

Changes proposed in this pull request:

  • remove some dead code;
  • handle non-decimal ASCII in headers properly;
  • handle comment strings that can start in the middle of a token and end with LF or CR;
  • make the exception types raised more consistent (although the resolution of issues raised in Add proper exception types #2339 and related is still pending);
  • add two tests; change the exception type expected in other two.

src/PIL/PpmImagePlugin.py Outdated Show resolved Hide resolved
src/PIL/PpmImagePlugin.py Outdated Show resolved Hide resolved
src/PIL/PpmImagePlugin.py Outdated Show resolved Hide resolved
Tests/test_file_ppm.py Outdated Show resolved Hide resolved
Copy link
Contributor

@gofr gofr left a comment

Choose a reason for hiding this comment

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

I'm just a random contributor so I don't dare to "Approve", but apart from the minor notes I added this looks good to me. 👍

@Piolie
Copy link
Contributor Author

Piolie commented Jan 6, 2021

I hurried to push and forgot to lint and a raise from None. I'll fix that and the other tests as noted by @gofr.

- add test for maxcolors;
- extend coverage for wrong magic number;
- fix linting.
Co-authored-by: Andrew Murray <3112309+radarhere@users.noreply.github.com>

with pytest.raises(ValueError):
with Image.open(path):
pass
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 quite on board with this one. The idea here is that b"\x00" is not caught by

if c > b"\x79":
raise ValueError("Expected ASCII value, found binary")

even though \x00 is not greater than \x79, and only for it to be raised as a different ValueError later when it turns out that b"128\x00" is not an integer.

There isn't a valid scenario where the token/magic needs to be something other than a number or a letter. I've pushed a commit to change this test into a check that a ValueError is raised when a token is not an integer.

@radarhere radarhere merged commit 6d432f2 into python-pillow:main Mar 4, 2022
@radarhere radarhere changed the title Improve handling of PPM headers Improve PPM header handling Mar 4, 2022
@radarhere radarhere changed the title Improve PPM header handling Improved handling of PPM header Mar 4, 2022
@Piolie Piolie deleted the PPMheaders branch May 8, 2022 04:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants