pypdf icon indicating copy to clipboard operation
pypdf copied to clipboard

BUG: Don't close stream passed to PdfWriter.write()

Open alexaryn opened this issue 1 year ago • 2 comments

Closes #2905

alexaryn avatar Oct 18 '24 22:10 alexaryn

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 96.39%. Comparing base (9e0fce7) to head (c46072d). Report is 107 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2909   +/-   ##
=======================================
  Coverage   96.39%   96.39%           
=======================================
  Files          52       52           
  Lines        8730     8741   +11     
  Branches     1590     1590           
=======================================
+ Hits         8415     8426   +11     
  Misses        186      186           
  Partials      129      129           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Oct 18 '24 22:10 codecov[bot]

I've created PR #2913 that also deals with Context manager. Consistency should be checked

pubpub-zz avatar Oct 20 '24 17:10 pubpub-zz

@stefan6419846 let me know how you'd like to proceed

alexaryn avatar Oct 22 '24 20:10 alexaryn

While I doubt that many users really rely on it, the deprecation process mentioned in https://github.com/py-pdf/pypdf/pull/2909/files#r1808253593 is still necessary as we are replacing a public attribute.

stefan6419846 avatar Oct 23 '24 04:10 stefan6419846

The Windows CI test looks like a spurious failure due to a file missing from cache. I'm not sure what to do with the test coverage issues, as the lines cited are after the deprecation logic.

alexaryn avatar Oct 29 '24 19:10 alexaryn

Do not worry about the Windows CI - we will have a look at this. Windows does not use the cache for different reasons, but we are currently evaluating alternatives to the usually failing arXiv downloads while avoiding copyright issues.

For the coverage, we tend to have a corresponding test which ensures the deprecation message being issued correctly (at least for past deprecations). This should work here as well by trying to read and write the value in one dedicated test function and looking into caplog.

stefan6419846 avatar Oct 29 '24 19:10 stefan6419846

@stefan6419846 I tried to rely on caplog alone, but the presence of the warning seemed to fail the test. So, I used pytest.warns() which allows me to validate the message and keeps the test from failing. For some reason, the deprecation message does not end up in the caplog; so, I'm not checking it.

alexaryn avatar Oct 30 '24 18:10 alexaryn