Skip to content

STY: Remove variable check_crlf_space #3096

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

j-t-1
Copy link
Contributor

@j-t-1 j-t-1 commented Jan 30, 2025

No description provided.

Copy link

codecov bot commented Jan 30, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.60%. Comparing base (3c72462) to head (514982d).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3096      +/-   ##
==========================================
- Coverage   96.60%   96.60%   -0.01%     
==========================================
  Files          53       53              
  Lines        8979     8974       -5     
  Branches     1659     1659              
==========================================
- Hits         8674     8669       -5     
  Misses        183      183              
  Partials      122      122              

☔ 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.

@stefan6419846
Copy link
Collaborator

Doesn't this decrease readability/overview?

@j-t-1
Copy link
Contributor Author

j-t-1 commented Jan 31, 2025

Whether to check_crlf_space depends on if operator in ( b"Td", b"Tm", b"T*", b"Tj"). So the variable is recording something we already know and thus decreases readability.

@stefan6419846 stefan6419846 added the on-hold PR requests that need clarification before they can be merged.A comment must give details label Feb 2, 2025
pypdf/_page.py Outdated
@@ -2043,7 +2041,7 @@ def process_operation(operator: bytes, operands: List[Any]) -> None:
else:
return

if check_crlf_space:
if operator in ( b"Td", b"Tm", b"T*", b"Tj"):
Copy link
Collaborator

Choose a reason for hiding this comment

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

There is a superfluous whitespace here. Additionally: Will operator always be a primitive type which would allow converting the right-hand side to a set for faster lookups?

Copy link
Contributor Author

@j-t-1 j-t-1 Apr 16, 2025

Choose a reason for hiding this comment

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

Apart from this line they are always bytes literals:

process_operation(operator, operands)

operator comes from content.operations:

for operands, operator in content.operations:

content comes from:

pypdf/pypdf/_page.py

Lines 1863 to 1867 in 3c72462

content = (
obj[content_key].get_object() if isinstance(content_key, str) else obj
)
if not isinstance(content, ContentStream):
content = ContentStream(content, pdf, "bytes")

operator is stored as bytes in ContentStream:

self._operations: List[Tuple[Any, bytes]] = []

So operator will always be a primitive type (bytes).

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
on-hold PR requests that need clarification before they can be merged.A comment must give details
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants