Skip to content

Cleanup string formatting. #4587

New issue

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

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

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Cleanup string formatting. #4587

wants to merge 1 commit into from

Conversation

knotapun
Copy link

@knotapun knotapun commented Jul 2, 2025

This commit attempts to clean out old cruft, to make room for new cruft. This is clearly not the most pressing issue in the world, however I think it's far more legible.

This change consists of:

  • Replacing most %-strings with f-strings.
  • Cleanup a few areas where repeated string formatting logic was used, by using a loop instead.

Copy link
Contributor

github-actions bot commented Jul 2, 2025

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

@knotapun
Copy link
Author

knotapun commented Jul 2, 2025

I have read the CLA Document and I hereby sign the CLA

@knotapun
Copy link
Author

knotapun commented Jul 2, 2025

recheck

github-actions bot added a commit that referenced this pull request Jul 2, 2025
@knotapun
Copy link
Author

knotapun commented Jul 3, 2025

Read through the action result, and it looks like it wasn't happy about an extra line, and I forgot to wrap arguments for the SQLite call in the sample, so it did a different form of injection. Ironic. At least it failed fast?

Copy link
Collaborator

@jamie-lemon jamie-lemon left a comment

Choose a reason for hiding this comment

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

diffs in RST files look good to me

@JorjMcKie JorjMcKie requested a review from Copilot July 16, 2025 16:42
Copilot

This comment was marked as outdated.

@knotapun
Copy link
Author

  • The raise syntax is invalid here. It should be raise ValueError(f"max. code point found: {ord(max(text))}, increase limit") without the colon after ValueError.
               raise ValueError:(f"max. code point found: {ord(max(text))}, increase limit")

src/utils.py:1640

The syntax error is real, but I didn't introduce it. That's easy to fix though.

@knotapun
Copy link
Author

Everything copilot complained about, (Except the pass thing, which it was wrong about), I fixed. I don't think the dictionary thing makes a difference speed wise, but I think it's stylistically better.

@JorjMcKie JorjMcKie requested a review from Copilot July 16, 2025 21:23
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR standardizes string formatting by replacing legacy %- and % formatting with Python f-strings and removes duplicated formatting logic by introducing loops.

  • Replaced most %-based string interpolations with f-strings.
  • Consolidated repeated /First, /Last, etc. PDF outline key formatting into a loop.
  • Updated documentation and sample scripts to demonstrate f-string usage.

Reviewed Changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/utils.py Replaced %-strings with f-strings and refactored repeated outline key formatting into a loop
src/main.py Updated CLI messages and sys.exit calls from % formatting to f-strings
src/init.py Converted annotation reprs and CSS style methods to use f-strings
docs/tutorial.rst Updated code example for saving page images to f-strings
docs/the-basics.rst Changed image save example to f-strings
docs/samples/text-lister.py Swapped %-formatting for f-strings and streamlined prints
docs/samples/new-annots.py Replaced % formatting with f-strings in annotation text and save calls
docs/samples/multiprocess-render.py Updated process print statements to use f-strings
docs/samples/filmfestival-sql.py Switched to parameterized queries and f-strings for printing
docs/recipes-text.rst Updated footer and logging examples to f-strings
docs/recipes-low-level-interfaces.rst Converted object print formatting to f-strings
docs/recipes-journalling.rst Updated journalling examples with f-strings
docs/recipes-images.rst Changed pixmap save and timing prints to f-strings
docs/recipes-common-issues-and-their-solutions.rst Updated conversion print statements to f-strings
docs/functions.rst Converted error raise formatting to f-strings
docs/font.rst Updated codepoint print example to f-strings
docs/document.rst Changed page-loop prints to f-strings
docs/app4.rst Updated pix.save example to f-strings
Comments suppressed due to low confidence (3)

src/utils.py:1622

  • [nitpick] Variable name shadows the built-in function name. Consider renaming it to something more descriptive like outline_key or link_key.
    name_key_pairs = {"first": "First", "last": "Last", "next": "Next", "parent": "Parent", "prev": "Prev"}

src/utils.py:5063

  • This f-string uses unescaped double quotes inside double quotes, causing a syntax error. Use single quotes for the key, e.g., f"{label['startpage']}<<{pref_part}{styl_part}{page_part}>>".
        return f"{label["startpage"]}<<{pref_part}{styl_part}{page_part}>>"

src/utils.py:1627

  • Catching Exception broadly can mask unexpected errors. Consider catching more specific exceptions (e.g., KeyError or IndexError) when accessing ol[name].
        try:

@jamie-lemon
Copy link
Collaborator

@knotapun Would it be possible to squash your commits here?

Fix syntax error, move dict because copilot nitpick.
Use sql parameterization.
Some loops inserted where repeated logic was used.
@knotapun
Copy link
Author

They're now squashed into one commit. Git is my biggest weakness, sorry if this is not the appropriate way to do this.

@jamie-lemon
Copy link
Collaborator

Looks good to me, my "git-fu" is also pretty weak when it comes to this sort of thing!

As far as a I am concerned this PR is fine, but I think we need to spend some time double checking and testing that none of our tests are broken (which I doubt). So don't be surprised if it takes a few days :)

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