Skip to content

Conversation

j-t-1
Copy link
Contributor

@j-t-1 j-t-1 commented Aug 13, 2025

No description provided.

@j-t-1
Copy link
Contributor Author

j-t-1 commented Aug 13, 2025

@stefan6419846 the changes in this PR caused an error with pillow import. Is an obvious fix?

@stefan6419846
Copy link
Collaborator

Do not import from _xobj_image_helpers globally, but only locally as pypdf.filters did.

@j-t-1
Copy link
Contributor Author

j-t-1 commented Aug 13, 2025

Do not import from _xobj_image_helpers globally, but only locally as pypdf.filters did.

Sorry still do not understand; which function(s) need the local imports?

Is the reason for importing locally so test_image_without_pillow works, otherwise testing without pillow would not work?

@stefan6419846
Copy link
Collaborator

An installation without Pillow should work until the user actually tries to export images using PageObject.images, thus we have to ensure that we do not accidentally import it when not required.

Copy link

codecov bot commented Sep 23, 2025

Codecov Report

❌ Patch coverage is 97.67442% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.07%. Comparing base (d40f359) to head (a13eac6).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pypdf/_xobj_image_helpers.py 97.56% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3434   +/-   ##
=======================================
  Coverage   97.07%   97.07%           
=======================================
  Files          56       56           
  Lines        9614     9614           
  Branches     1742     1741    -1     
=======================================
  Hits         9333     9333           
  Misses        168      168           
  Partials      113      113           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@j-t-1
Copy link
Contributor Author

j-t-1 commented Sep 23, 2025

@stefan6419846 please could you provide guidance for the coverage?

@stefan6419846
Copy link
Collaborator

  • The Pillow import always had issues and might be ignorable or something to further investigate.
  • img = Image.frombytes(mode, img1.size, img1.tobytes()) has always been untested, but might be something where we can generate a corresponding sample image setup on the fly.
  • img = img.convert("RGB") has always been untested, but might be something where we can generate a corresponding sample image setup on the fly.

@stefan6419846 stefan6419846 added the needs-test A test should be added before this PR is merged. label Sep 23, 2025
@j-t-1
Copy link
Contributor Author

j-t-1 commented Sep 23, 2025

Unsure how to do those tests. I would like to simplify the code in _xobj_image_helpers.py in a new PR, and then adding the tests may be easier. Do that in parallel?

@stefan6419846
Copy link
Collaborator

Unsure how to do those tests.

Analyze the code execution flow and set up the artificial data accordingly. There wouldn't be much of a difference when I would do this compared to you, except that you are already familiar with the specific code.

I would like to simplify the code in _xobj_image_helpers.py in a new PR, and then adding the tests may be easier. Do that in parallel?

How do you imagine this? We would still have to merge on PR first. What are your refactoring plans? In general (as we already discussed for the page-specific JavaScript actions), please always outline such (non-trivial) changes beforehand to discuss them before attempting to do this, ideally in a proper issue, filling the provided issue template. (In theory, even this PR should have a corresponding issue for clarification first.)

@j-t-1
Copy link
Contributor Author

j-t-1 commented Sep 23, 2025

The refactoring is likely small, to make it more readable. Refactoring may be too strong a word here. And yes, may not help with the tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-test A test should be added before this PR is merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants