ENH: context manager for PdfReader

[tibor-reiss]

Closes #2665

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.03%. Comparing base (08731fa) to head (cc3d491).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2666      +/-   ##
==========================================
+ Coverage   95.01%   95.03%   +0.01%     
==========================================
  Files          50       50              
  Lines        8352     8372      +20     
  Branches     1673     1674       +1     
==========================================
+ Hits         7936     7956      +20     
  Misses        258      258              
  Partials      158      158              

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

[pubpub-zz]

Can you please add a test for coverage

[stefan6419846]

I am not completely sure: Which effect should be the goal of this change? The reading still happens in the __init__ method and there a no resources which would require closing on exit where a context manager would make sense?

[tibor-reiss]

@stefan6419846 e.g. self.stream is not closed which could result in a memory leak. Additionally, in the 2nd commit, I added some other attributes which have similar issue.

You are right though that __init__ contains the read method - should this be moved to __enter__? However, that might break some existing code.

[MasterOdin]

I think that self.stream should only be closed as part of __exit__ if it was created as part of __init__. If a user passes in a stream to PdfReader, then closing it may not be desired.

You are right though that __init__ contains the read method - should this be moved to enter? However, that might break some existing code.

Unless the desire was to mandate that people must use a context manager, having __enter__ only return self and do nothing else (and leave all the work to __init__) is super common. You would also potentially want to have a close method which __exit__ calls so that users who don't use a context manager can also close out their reader and avoid a memory leak.

[tibor-reiss]

close() was also implemented in _merger.py - thanks for the hint @MasterOdin.