refactor: convert printf-style formats to f-strings - #48
Merged
Merged
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
blaipr
force-pushed
the
refactor/printf-formats
branch
from
September 12, 2026 00:50
3a79075 to
e11274c
Compare
This was referenced Sep 12, 2026
UP031 to reach .format(), then f-strings at those sites only, so this does not overlap the str.format conversion in ctrliq#41. Four are left as % because their argument is a float that %d truncates and {:d} rejects. Formatted with black, which is what CI runs today.
blaipr
added a commit
to blaipr/ascender-kit
that referenced
this pull request
Sep 13, 2026
The last four printf-style formats in the package, left out of ctrliq#48 because Ruff refuses them and is right to. ```python group_by_10s = 'group-%07dX.example.com' % (n / 10) ``` `n / 10` is true division, so the argument is a float. `%07d` truncates it; `{:07d}` raises `ValueError: Unknown format code 'd' for object of type 'float'`. A mechanical conversion breaks, which is why the automatic fix is unavailable. The real problem is the division, not the format. These are group names bucketing hosts by ten, hundred and thousand, so the intent is integer division throughout, and `%d` was quietly papering over a float. Switched to `//`, and then the f-string conversion is trivial: ```python group_by_10s = f'group-{n // 10:07d}X.example.com' ``` `hostname` came along with them: `n` is already an integer there, so it converts directly. Checked exhaustively rather than by argument. Every `n` from 0 to 99,999, all four formats, old expression against new: ``` mismatches over 100000 hosts, 4 formats each: 0 ``` That is the whole range this function is used over, since `n` comes from `range(nhosts)` and is never negative, which is the only case where truncation and floor disagree. `json_inventory(25)` still produces the same groups and hostnames. With this the package has no printf-style formats left: `ruff check --select UP031` reports `All checks passed!`. Note for whoever merges: ctrliq#48 also edits this file, so whichever lands second needs a rebase. Verified with `black --check`, `flake8` and the unit suite, 355 passing.
blaipr
force-pushed
the
refactor/printf-formats
branch
from
September 13, 2026 09:03
e11274c to
1aeade5
Compare
cigamit
approved these changes
Sep 13, 2026
cigamit
pushed a commit
that referenced
this pull request
Sep 13, 2026
The last four printf-style formats in the package, left out of #48 because Ruff refuses them and is right to. ```python group_by_10s = 'group-%07dX.example.com' % (n / 10) ``` `n / 10` is true division, so the argument is a float. `%07d` truncates it; `{:07d}` raises `ValueError: Unknown format code 'd' for object of type 'float'`. A mechanical conversion breaks, which is why the automatic fix is unavailable. The real problem is the division, not the format. These are group names bucketing hosts by ten, hundred and thousand, so the intent is integer division throughout, and `%d` was quietly papering over a float. Switched to `//`, and then the f-string conversion is trivial: ```python group_by_10s = f'group-{n // 10:07d}X.example.com' ``` `hostname` came along with them: `n` is already an integer there, so it converts directly. Checked exhaustively rather than by argument. Every `n` from 0 to 99,999, all four formats, old expression against new: ``` mismatches over 100000 hosts, 4 formats each: 0 ``` That is the whole range this function is used over, since `n` comes from `range(nhosts)` and is never negative, which is the only case where truncation and floor disagree. `json_inventory(25)` still produces the same groups and hostnames. With this the package has no printf-style formats left: `ruff check --select UP031` reports `All checks passed!`. Note for whoever merges: #48 also edits this file, so whichever lands second needs a rebase. Verified with `black --check`, `flake8` and the unit suite, 355 passing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The other half of the string formatting sweep: the 32 printf-style
%formats across 12 files, which Ruff flags as UP031 and offers no safe fix for. #41 covers thestr.format()calls, and the two do not overlap: this pull request touches no pre-existing.format()call and adds none.Applied in two steps, because UP031 rewrites
%to.format()rather than to an f-string, which would have left this adding calls the other pull request immediately wants to remove:ruff check --select UP031 --fix --unsafe-fixes, converting 28 of the 32.Run from a throwaway Ruff configuration rather than one added to the repository, so this carries no tooling change and merges on its own. Formatted with
black, which is whattox -e lintand the CI job run today.The unsafe marking on UP031 is about one case:
'%s' % xformats a tuple element-wise where'{}'.format(x)formats the tuple. Every converted site was read for that. They are assertion messages, log lines, inventory file fragments andargparseerror text, and none of the substituted values can be a tuple.Four are left as
%on purpose, all inascender/inventory.py, and Ruff is right to refuse them:n / 10is a float on Python 3, and%07dtruncates it where{:07d}raises:So converting these is a behaviour change dressed as a style fix. They want
//rather than/, which is a correctness question and belongs in its own change.Verified with
black --check,flake8and the unit suite, 355 passing.On the formatter. This branch is cut from
main, wheretox -e lintand the CI job still runblackandflake8; #33 is the pull request that switches them to Ruff and is not merged. Soblack --checkis what CI would actually run here, and validating against Ruff instead would be validating against a toolchain the base branch does not have.Worth knowing for the rebase, though: the two formatters disagree about wrapping a long
assert. Black parenthesises the condition, Ruff parenthesises the message. Onmainthey already differ over 6 files, and the three long assertion messages converted by hand here make it 9. Nothing to fix in this branch, since #33 reformats the whole tree withruff formatand settles all of them at once, but whichever of the two lands second wants aruff formatpass as part of the rebase.