Skip to content

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.

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.

Piolie added 2 commits January 6, 2021 14:46
- add test for maxcolors;
- extend coverage for wrong magic number;
- fix linting.

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.

3 participants