Skip to content

refactor: ontext managers for PDF - #22

Open
gustavo-szesz wants to merge 3 commits into
NP-compete:mainfrom
gustavo-szesz:refactor--context-managers-for-PDF-files
Open

refactor: ontext managers for PDF#22
gustavo-szesz wants to merge 3 commits into
NP-compete:mainfrom
gustavo-szesz:refactor--context-managers-for-PDF-files

Conversation

@gustavo-szesz

@gustavo-szesz gustavo-szesz commented Mar 3, 2026

Copy link
Copy Markdown

Description

Brief description of the changes in this PR.

Related Issue

Fixes #(issue number)

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Screenshots (if applicable)

Add screenshots to help explain your changes.

Additional Notes

Any additional information that reviewers should know.

@NP-compete

Copy link
Copy Markdown
Owner

Thanks for the PR! 🎉

The lint check is failing because two files need formatting. Please run:

ruff format src/ tests/

This will reformat:

  • src/pageindex/pdf/parser.py
  • src/pageindex/utils.py

Then commit and push the changes. The CI should pass after that.

Comment thread .gitignore Outdated
Comment thread src/pageindex/utils.py
pdf_name = meta.title if meta and meta.title else "Untitled"
return sanitize_filename(pdf_name)
finally:
if hasattr(pdf_reader, "stream") and hasattr(pdf_reader.stream, "close"):

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Closing pdf_reader.stream here will close the caller's BytesIO. They won't be able to reuse it after calling this function. Better to let the caller own the lifecycle.

Comment thread src/pageindex/utils.py
return meta.title if meta and meta.title else "Untitled"
else:
pdf_reader = PyPDF2.PdfReader(str(pdf_path))
try:

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This else branch is unreachable. The type hint is str | Path, so the isinstance check above always passes. Safe to remove.

Comment thread src/pageindex/utils.py
meta = pdf_reader.metadata
pdf_name = meta.title if meta and meta.title else "Untitled"
return sanitize_filename(pdf_name)
finally:

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hasattr(pdf_reader, "stream") pattern relies on PyPDF2 internals. The with open() approach you used on lines 86-89 is cleaner. Consider using that consistently.

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.

2 participants