-
Notifications
You must be signed in to change notification settings - Fork 629
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
base: main
Are you sure you want to change the base?
Conversation
All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
recheck |
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? |
There was a problem hiding this 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
The syntax error is real, but I didn't introduce it. That's easy to fix though. |
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. |
There was a problem hiding this 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 functionname
. Consider renaming it to something more descriptive likeoutline_key
orlink_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
orIndexError
) when accessingol[name]
.
try:
@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.
They're now squashed into one commit. Git is my biggest weakness, sorry if this is not the appropriate way to do this. |
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 :) |
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: