BUG: Fix conversion from 1 to LA

[pubpub-zz]

Closes #2165
Closes #2176

[pubpub-zz]

@MartinThoma
During pre-commit I've got the following issues reported:

pypdf\filters.py:795:5: C901 _xobj_to_image is too complex (55 > 54)
pypdf\filters.py:795:5: PLR0915 Too many statements (178 > 176)
tests\test_writer.py:1607:9: S603 subprocess call: check for execution of untrusted input
Found 4 errors (1 fixed, 3 remaining).

[codecov]

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (05f2a65) 94.25% compared to head (c0aa032) 94.25%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2175   +/-   ##
=======================================
  Coverage   94.25%   94.25%           
=======================================
  Files          42       42           
  Lines        7556     7558    +2     
  Branches     1487     1488    +1     
=======================================
+ Hits         7122     7124    +2     
  Misses        266      266           
  Partials      168      168           

Files Changed	Coverage Δ
pypdf/filters.py	94.54% <100.00%> (+0.02%)	⬆️

☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.

[pubpub-zz]

@MartinThoma
this PR is all yours. Can you fix this issue ?

[stefan6419846]

I cannot see the third violation in CI (this should be related to the subprocess call for the watermark rendering, where there is an non-fixable error raised regardless of how the call is made).

For the other issues, see #2108. You should either refactor the code to reduce complexity or be able to justify why the increased complexity is necessary and cannot be avoided.

[pubpub-zz]

I cannot see the third violation in CI (this should be related to the subprocess call for the watermark rendering, where there is an non-fixable error raised regardless of how the call is made).

For the other issues, see #2108. You should either refactor the code to reduce complexity or be able to justify why the increased complexity is necessary and cannot be avoided.

the refactoring required is too important to be part of this PR : this will require an idenpendant PR. about the subprocess it should be part of an independant PR too for traceeability

[stefan6419846]

You are not asked to do the refactoring of the complete code, but only for the corresponding section. I do not really see why this cannot be part of this PR - the chosen approach is to slowly refactor the sections which are changed anyway to not do everything at once. And as you are working on the corresponding code section, you already show that you understand enough of it to be able to safely refactor it, assuming that our test coverage is sufficient.

And as already mentioned: The subprocess violation you see should not even occur and cannot be seen in the CI style checks, as it should be ignored due to the noqa marker in the correspondig line already. If you experience issues on your personal device with pre-commit stuff, there seems to be some issue with your setup or with the pre-commit handling (where I do not have any real knowledge about).

[MartinThoma]

The two style issues we currently see are

pypdf/filters.py:795:5: C901 `_xobj_to_image` is too complex (55 > 54)
pypdf/filters.py:795:5: PLR0915 Too many statements (178 > 176)

I'll check how we can change that :-)

[MartinThoma]

Good work @pubpub-zz 👍

[MartinThoma]

I like that the PR is clean / minimal. It was a good decision to not include refactoring in it, as that would have made the review way more complex. I'll take care of that now

[stefan6419846]

This probably is a matter of taste: In general, projects prefer the current contributor to fix all issues introduced with the current PR from what I have seen in the past (my GitHub activity is non-representative here). Maybe I am a bit opinionated about this as well due to doing a lot of code quality stuff.

In the end, it is up to you as the maintainer to decide on how you handle this, as long as this is a consistent behaviour which is applied equally to all changes.

[MartinThoma]

Typically, I would agree that issues should be fixed within the PR. However, there are some exceptions:

• Value trade-off: If the value added by the PR is bigger than the issue it introduced, I don't want to block the PR.
• PR readability: I like refactorings to be in separate PRs as it makes them so much easier to understand.