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? |
jamie-lemon
left a comment
There was a problem hiding this comment.
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.
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
nameshadows the built-in functionname. Consider renaming it to something more descriptive likeoutline_keyorlink_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
Exceptionbroadly can mask unexpected errors. Consider catching more specific exceptions (e.g.,KeyErrororIndexError) 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 :) |
julian-smith-artifex-com
left a comment
There was a problem hiding this comment.
Looks good.
Unfortunately the code on main has been moved around, so i'm not sure this will rebase on top of main.
| >>> vuc = font.valid_codepoints() | ||
| >>> for i in vuc: | ||
| print("%04X %s (%s)" % (i, chr(i), font.unicode_to_glyph_name(i))) | ||
| >>> print(f"{i:04X} {chr(i)} ({font.unicode_to_glyph_name(i)})") |
There was a problem hiding this comment.
Minor quibble - this line should probably use a ... prefix instead of >>> .
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: