sprintf.eo #4771
sprintf.eo #4771
Conversation
WalkthroughIntroduces a new pure EO implementation of sprintf that parses format strings and generates formatted output. The object handles format specifiers (%s, %d, %f), literal %% escapes, and missing arguments with defaults. Includes comprehensive test cases demonstrating various formatting scenarios. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
eo-runtime/src/main/eo/org/eolang/sprintf.eo (1)
96-130: Test object should be in a separate test file.The
test_sprintfobject is co-located with the main implementation. Following standard practice, test code should reside in a separate test file (e.g., under a test directory) to keep production code clean and allow independent test execution.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
eo-runtime/src/main/eo/org/eolang/sprintf.eo(1 hunks)
🔇 Additional comments (2)
eo-runtime/src/main/eo/org/eolang/sprintf.eo (2)
114-130: The code is correct; stdout accepts single arguments.Each
stdoutcall in lines 114-130 is a separate statement within theseqblock. Thestdoutobject accepts a single[text]parameter, not multiple arguments. The indented formatting creates a sequence of distinctstdoutcalls, not multiple arguments to one call. No changes needed.
1-1: Remove or clarify the suspicious alias that conflicts with the local sprintf definition.This file defines a complete
sprintfimplementation in theorg.eolangpackage (line 10), yet it imports an alias toorg.eolang.tt.sprintf(line 1). This creates ambiguity about which sprintf is actually used. Additionally, based on the codebase structure, the correct import should beorg.eolang.txt.sprintf(nottt), as sprintf was moved from the root to the txt folder. Either remove the alias since this file defines its own implementation, or clarify why the external reference is needed.
| [] > sprintf | ||
| format > fmt | ||
| args > args |
There was a problem hiding this comment.
Critical: sprintf object lacks parameter definitions.
The object is defined with [] > sprintf (no parameters), but lines 11-12 reference format and args as if they were passed in. In EO, parameters should be declared in the brackets. This will cause undefined reference errors.
🔎 Proposed fix to add parameters
-[] > sprintf
- format > fmt
- args > args
+[format args] > sprintf
+ format > fmt
+ args > args📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [] > sprintf | |
| format > fmt | |
| args > args | |
| [format args] > sprintf | |
| format > fmt | |
| args > args |
🤖 Prompt for AI Agents
In eo-runtime/src/main/eo/org/eolang/sprintf.eo around lines 10 to 12, the
object is declared with no parameters ([] > sprintf) but then uses `format` and
`args`; update the object header to declare those parameters (e.g., include
`format` and `args` inside the brackets) so `format > fmt` and `args > args`
reference defined parameters; ensure the parameter names match the uses in the
body and adjust any naming consistency (fmt vs format) if needed.
| while. > @ | ||
| i.lt (fmt.length) | ||
| [i] > @ | ||
| if. > current_char | ||
| fmt.at i | ||
| current_char |
There was a problem hiding this comment.
Critical: Self-referential conditional and parameter shadowing.
Two issues here:
- Line 23: The while loop parameter
[i]shadows the outermemory > iobject, which will break the loop logic since the inneriwon't refer to the mutable counter. - Lines 24-26: The
if.statement referencescurrent_charin its own definition, which is self-referential and invalid.
The correct structure should extract the character first, then use it:
fmt.at i > current_char
if.
current_char.eq "%"
...
🤖 Prompt for AI Agents
In eo-runtime/src/main/eo/org/eolang/sprintf.eo around lines 21 to 26, the inner
parameter `[i]` in the while block shadows the outer mutable `memory > i`
counter and the `if.` uses `current_char` in its own definition
(self-referential). Remove the inner `[i]` parameter so the while body mutates
the outer `i`, first evaluate the character with `fmt.at i > current_char`, then
call `if.` using a proper comparison like `current_char.eq "%"` (or other
predicate) instead of referencing `current_char` inside the `if.` definition.
| seq > @ | ||
| output.write > @ | ||
| output.concat "%" | ||
| i.write > @ | ||
| i.add 2 |
There was a problem hiding this comment.
Multiple > @ decorations within seq block may cause unexpected behavior.
Within the seq block, multiple expressions are decorated with > @. In EO, > @ indicates the "phi" attribute (return value). Having multiple > @ decorations in sequence is ambiguous—typically only the last should be the return value, and intermediate steps should not be decorated.
The pattern repeats throughout lines 44-55, 58-69, 72-83, and 84-88. Consider removing > @ from intermediate statements within seq blocks.
🤖 Prompt for AI Agents
In eo-runtime/src/main/eo/org/eolang/sprintf.eo around lines 37-41, multiple
expressions inside the seq block are annotated with "> @" which makes the return
(phi) ambiguous; remove the "> @" decoration from intermediate statements (e.g.,
output.write and i.write) so only the final intended expression in each seq has
"> @". Apply the same change to the other reported ranges (44-55, 58-69, 72-83,
84-88), leaving a single "> @" on the last expression of each seq and deleting
it from preceding statements.
| memory > result3 | ||
| sprintf | ||
| "Pi: %.2f" | ||
| (3.14159 0.0) |
There was a problem hiding this comment.
Float precision specifier %.2f is not implemented.
The test uses format string "Pi: %.2f" which includes a precision specifier (.2), but the implementation at lines 70-83 only handles bare %f. The code will fail to match next_char.eq "f" because next_char will be . (the character after %), not f.
Either:
- Update the test to use
%fwithout precision:"Pi: %f" - Or implement precision parsing in the format specifier handling
🤖 Prompt for AI Agents
In eo-runtime/src/main/eo/org/eolang/sprintf.eo around lines 105-108, the format
string "Pi: %.2f" uses a precision specifier (".2") but the current parser only
expects a bare "%f" so it will see '.' as the next_char and fail; fix by either
(A) updating the test to use "Pi: %f" (remove the precision) OR (B) implement
precision parsing: after consuming '%' check for an optional '.' followed by one
or more digit chars, parse that integer as precision, then continue to match the
type char 'f' and format the float with the parsed precision when constructing
the output.
🚀 Performance AnalysisAll benchmarks are within the acceptable range. No critical degradation detected (threshold is 100%). Please refer to the detailed report for more information. Click to see the detailed report
|
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.