Fix padding bug#48
Conversation
Fixes a bug where padding would not be calculated correctly in cases where a padding character was not provided
donatj
force-pushed
the
fix/padding-count-bug
branch
from
9044f98 to
9f587c8
Compare
There was a problem hiding this comment.
Pull request overview
Fixes parsing of printf padding width when no padding character flag is provided, and extends integration coverage around validity/invalid lexemes.
Changes:
- Parse a leading integer after
%as a pad width when it is not a positional argument ($). - Always attempt to read a pad width after flag parsing (instead of only when a pad char flag was set).
- Expand
ParserTestdata provider to include validity expectations and additional format cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/Parser.php |
Adjusts width parsing logic so widths are recognized even when pad char isn’t specified. |
test/Integration/ParserTest.php |
Adds validity assertions and new/updated integration cases for padding/width scenarios. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $this->assertSame( | ||
| ($invalid === null), | ||
| !($valid === false), | ||
| sprintf( |
There was a problem hiding this comment.
$valid is documented as bool|string and the provider supplies canonical strings in a couple cases, but the test only treats it as a boolean validity flag ($valid === false). This makes the data provider contract misleading and leaves the canonical string values unused.
Either assert the canonical string when $valid is a string (if canonicalization is a requirement), or change the provider/doc/type expectations to a simple boolean validity indicator.
| $lexer->rewind(); | ||
| $peek = $lexer->peek(); | ||
| if( ctype_digit($peek->getString()) ) { | ||
| $padWidth = $this->eatInt($lexer); | ||
| } | ||
|
|
||
| $next = $lexer->next(); | ||
|
|
There was a problem hiding this comment.
lexSprintf rewinds and re-parses a width unconditionally. If the format string ends immediately after an initial width (e.g. "%10"), $next is EOF, rewind() moves back onto the last digit, and eatInt() overwrites $padWidth with only that trailing digit (10 becomes 0). This makes the parsed pad width incorrect for EOF-after-width cases.
Consider only rewinding/parsing width when the current $next is a digit (e.g. ctype_digit($next->getString())), and skip the rewind/width parse when $next is EOF or a non-digit.
There was a problem hiding this comment.
Applied in 93a467e. The unconditional rewind() is replaced with a check on $next (the token that broke the flag loop) — only rewinding and re-parsing the pad width when that token is itself a digit. EOF has length()=0 so never advances the position, meaning the old rewind() would step back onto the last digit of an already-parsed width and overwrite $padWidth with just that trailing digit. A test case for %10 (no flags, EOF after width) is also added.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Agent-Logs-Url: https://github.com/donatj/printf-parser/sessions/47568dcd-c23b-457a-8dfe-ad18756b4b45 Co-authored-by: donatj <133747+donatj@users.noreply.github.com>
Fixes a bug where padding would not be calculated correctly in cases where a padding character was not provided