Repository navigation
Use strtr - #800
Use strtr#800divinity76 wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely identifies the main change: replacing existing placeholder substitutions with strtr(). Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
- Fix all pre-merge checks with AI
✨ Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
There was a problem hiding this comment.
🟡 Changes recommended
The applyFormat() change alters existing behavior for custom prefixes containing numbering tokens.
1 open finding
What changed in this PR
Replaces array-based str_replace() calls with strtr() in numbering formatting.
Changes:
- Simplifies date-placeholder replacement.
- Simplifies number-format replacement, but introduces a regression for token-bearing prefixes.
| File | Description |
|---|---|
Modules/Core/Support/NumberGenerator/AbstractNumberGenerator.php |
Replaces date placeholders using strtr(). |
Modules/Core/Models/Numbering.php |
Replaces numbering tokens using strtr(). |
🧠 Review effort: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| ]; | ||
|
|
||
| return str_replace(array_keys($replacements), array_values($replacements), $format); | ||
| return strtr($format, $replacements); |
|
@divinity76 thanks man! Keeper coming |
|
Hmm, I thought this was straight forward, but the AI review here have a point, The original implementation supported partial recursive transformation, but only by accident, and only when the deeper translation label comes after the outer translation label in the $translations array table, check this: @nielsdrost7 If we don't need to support recursive translation at all, then this PR is already fine. But if we want proper recursive translation support, where it does not matter what order the translation labels are defined in the array, we can do something like function strtr_recursive(string $str, array $table, int $maxDepth = 100): string
{
$depth = 0;
while ($depth < $maxDepth) {
$old = $str;
$str = strtr($str, $table);
if ($old === $str) {
return $str;
}
++$depth;
}
throw new \RuntimeException("translation exceeded max recursion depth ($maxDepth)");
} |
Thanks man! I'll take a look when I can. |
|
@divinity76 I've added you to the organization. As soon as you accept I can add you to a project which is an overview of the 200+ issues that are to be done (to-do) Pick the issue you like, assign yourself to it and see if you can tackle the issue you want to resolve. |
Use strtr.
2 places were using an awkward
str_replace(array_keys($replacements), array_values($replacements), $str);construction in places where strtr() should obviously have been used
(guess the original code author was unfamiliar with strtr)
Pull Request Checklist
Please check the following steps before submitting your PR. If any items are incomplete, consider marking it as
[WIP](Work in Progress).Checklist
docker compose run --rm cli php artisan test --exclude-group failing,troubleshootingDescription
Provide a brief description of the changes made in this pull request.
Related Issue(s)
List any related issues or discussions. If applicable, link to an accompanying thread on the forums.
Fixes # (example)
Motivation and Context
Why was this change necessary? Does it solve a problem or improve an existing feature? If this PR fixes an open issue, link to it here.
Issue Type (Check one or more)
Screenshots (If Applicable)
Attach relevant screenshots that demonstrate your changes.
Thank you for your contribution to InvoicePlane! We appreciate your time and effort.
Summary by CodeRabbit