Use Locale.ROOT with String.format which does or could render numbers#2320
Open
Use Locale.ROOT with String.format which does or could render numbers#2320
Conversation
Member
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (30)
✅ Files skipped from review due to trivial changes (20)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThis PR standardizes string formatting to use Locale.ROOT (locale-independent) across many String.format/printf call sites, with a few deliberate specifier adjustments; no public APIs or control-flow semantics were changed. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryThis PR systematically adds Key aspects of the change:
Confidence Score: 5/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["String.format call site"] --> B{Format string\ncontains numbers?}
B -- "Yes (%d, %f, %e, etc.)" --> C["Use String.format(Locale.ROOT, ...)"]
B -- "No (only %s, %b, %c)" --> D["Use String.format(...) — no locale needed"]
C --> E[Consistent output\nregardless of JVM locale]
D --> F[Locale-independent\nstring substitution]
E --> G["Machine messages, error text,\nversion strings, debug output"]
F --> G
style C fill:#2d8a4e,color:#fff
style D fill:#2d6ca8,color:#fff
style E fill:#1a5c33,color:#fff
style F fill:#1a4a7a,color:#fff
Last reviewed commit: f03bda4 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
spock-core/src/main/java/org/spockframework/runtime/extension/builtin/TimeoutInterceptor.java (2)
136-142: Inconsistent locale handling withprintfcalls.This method formats numbers with
%1.2fbut doesn't useLocale.ROOT, unlike the updatedString.formatcall at line 126. For consistency with the PR's objective, consider updating theseprintfcalls.♻️ Suggested fix for consistency
private void logUnsuccessfulInterrupt(String methodName, long now, long timeoutAt, long waitMillis, int unsuccessfulAttempts) { - System.err.printf( + System.err.printf(Locale.ROOT, "[spock.lang.Timeout] Method '%s' has not stopped after timing out %1.2f seconds ago - interrupting. Next try in %1.2f seconds.\n%n", methodName, TimeUtil.toSeconds(Duration.ofNanos(now - timeoutAt)), waitMillis / 1000d );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spock-core/src/main/java/org/spockframework/runtime/extension/builtin/TimeoutInterceptor.java` around lines 136 - 142, The System.err.printf call in logUnsuccessfulInterrupt formats floats without a fixed locale; update the call to use Locale.ROOT (similar to the String.format change) so numeric formatting is consistent across locales—locate the logUnsuccessfulInterrupt method and replace the System.err.printf invocation with a variant that supplies Locale.ROOT while preserving the same format string and argument order.
154-160: Inconsistent locale handling withprintfcall.Same issue as
logUnsuccessfulInterrupt- this method formats%1.2fwithoutLocale.ROOT.♻️ Suggested fix for consistency
private void logMethodTimeout(String methodName, double timeoutSeconds) { - System.err.printf( + System.err.printf(Locale.ROOT, "[spock.lang.Timeout] Method '%s' timed out after %1.2f seconds.%n", methodName, timeoutSeconds );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spock-core/src/main/java/org/spockframework/runtime/extension/builtin/TimeoutInterceptor.java` around lines 154 - 160, logMethodTimeout uses System.err.printf with a floating-point format without specifying Locale, causing locale-dependent formatting; update logMethodTimeout to call System.err.printf with Locale.ROOT as the first argument (mirroring logUnsuccessfulInterrupt) so the %1.2f is always formatted consistently, and ensure java.util.Locale is available/imported if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@spock-core/src/main/java/spock/util/concurrent/PollingConditions.java`:
- Around line 201-203: The code mixes locale-sensitive numeric formatting: some
numeric formats use Locale.ROOT (e.g., the String.format call that formats
elapsedTime/attempts) while other printf/String.format usages still rely on the
default locale when formatting "%1.2f". Locate the printf/String.format calls
that format numbers with "%1.2f" (the ones that reference
elapsedTime/attempts-style values) and change them to use Locale.ROOT (e.g.,
String.format(Locale.ROOT, ...) or printf(Locale.ROOT, ...)) so all numeric
formatting in PollingConditions uses Locale.ROOT consistently.
---
Nitpick comments:
In
`@spock-core/src/main/java/org/spockframework/runtime/extension/builtin/TimeoutInterceptor.java`:
- Around line 136-142: The System.err.printf call in logUnsuccessfulInterrupt
formats floats without a fixed locale; update the call to use Locale.ROOT
(similar to the String.format change) so numeric formatting is consistent across
locales—locate the logUnsuccessfulInterrupt method and replace the
System.err.printf invocation with a variant that supplies Locale.ROOT while
preserving the same format string and argument order.
- Around line 154-160: logMethodTimeout uses System.err.printf with a
floating-point format without specifying Locale, causing locale-dependent
formatting; update logMethodTimeout to call System.err.printf with Locale.ROOT
as the first argument (mirroring logUnsuccessfulInterrupt) so the %1.2f is
always formatted consistently, and ensure java.util.Locale is available/imported
if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec1fca92-9b83-4e75-8810-2aa66d783fb0
📒 Files selected for processing (30)
spock-core/src/main/java/org/spockframework/compiler/ErrorReporter.javaspock-core/src/main/java/org/spockframework/compiler/InvalidSpecCompileException.javaspock-core/src/main/java/org/spockframework/compiler/SpecRewriter.javaspock-core/src/main/java/org/spockframework/compiler/WhereBlockRewriter.javaspock-core/src/main/java/org/spockframework/mock/runtime/ByteBuddyMockFactory.javaspock-core/src/main/java/org/spockframework/mock/runtime/MockInteraction.javaspock-core/src/main/java/org/spockframework/runtime/DataVariablesIterationNameProvider.javaspock-core/src/main/java/org/spockframework/runtime/FailedSetEqualityComparisonRenderer.javaspock-core/src/main/java/org/spockframework/runtime/FailedStringComparisonRenderer.javaspock-core/src/main/java/org/spockframework/runtime/InvalidSpecException.javaspock-core/src/main/java/org/spockframework/runtime/SafeIterationNameProvider.javaspock-core/src/main/java/org/spockframework/runtime/SpockExecutionException.javaspock-core/src/main/java/org/spockframework/runtime/SpockRuntime.javaspock-core/src/main/java/org/spockframework/runtime/extension/ExtensionException.javaspock-core/src/main/java/org/spockframework/runtime/extension/builtin/TimeoutInterceptor.javaspock-core/src/main/java/org/spockframework/runtime/model/TextPosition.javaspock-core/src/main/java/org/spockframework/util/Assert.javaspock-core/src/main/java/org/spockframework/util/InternalSpockError.javaspock-core/src/main/java/org/spockframework/util/IoUtil.javaspock-core/src/main/java/org/spockframework/util/VersionNumber.javaspock-core/src/main/java/spock/config/ConfigurationException.javaspock-core/src/main/java/spock/lang/Snapshotter.javaspock-core/src/main/java/spock/mock/AutoAttachExtension.javaspock-core/src/main/java/spock/util/concurrent/AsyncConditions.javaspock-core/src/main/java/spock/util/concurrent/BlockingVariable.javaspock-core/src/main/java/spock/util/concurrent/PollingConditions.javaspock-spring/src/main/java/org/spockframework/spring/mock/DelegatingInterceptor.javaspock-spring/src/main/java/org/spockframework/spring/mock/SpockDefinition.javaspock-spring/src/main/java/org/spockframework/spring/mock/SpockMockPostprocessor.javaspock-spring/src/main/java/org/spockframework/spring/mock/SpyDefinition.java
Comment on lines
+201
to
+203
| String msg = String.format(Locale.ROOT, "Condition not satisfied after %1.2f seconds and %d attempts", elapsedTime / 1000d, attempts); | ||
| if (timeoutMessage != null) { | ||
| msg = String.format(Locale.ENGLISH, "%s: %s", msg, GroovyRuntimeUtil.invokeClosure(timeoutMessage, testException)); | ||
| msg = String.format("%s: %s", msg, GroovyRuntimeUtil.invokeClosure(timeoutMessage, testException)); |
There was a problem hiding this comment.
LGTM on these lines, but inconsistent locale handling elsewhere in the file.
Lines 201-203 are correct: Locale.ROOT is properly used for the numeric formatting on line 201, and line 203 only uses %s specifiers.
However, the printf calls at lines 137-142 and 155-159 format numbers using %1.2f without Locale.ROOT, which is inconsistent with this PR's objective.
,
🔧 Suggested fixes for consistency
At lines 137-142:
- System.err.printf(
- "[spock.lang.Timeout] Method '%s' has not stopped after timing out %1.2f seconds ago - interrupting. Next try in %1.2f seconds.\n%n",
+ System.err.printf(Locale.ROOT,
+ "[spock.lang.Timeout] Method '%s' has not stopped after timing out %1.2f seconds ago - interrupting. Next try in %1.2f seconds.%n",
methodName,At lines 155-159:
- System.err.printf(
- "[spock.lang.Timeout] Method '%s' timed out after %1.2f seconds.%n",
+ System.err.printf(Locale.ROOT,
+ "[spock.lang.Timeout] Method '%s' timed out after %1.2f seconds.%n",
methodName,Note: The suggested fixes reference TimeoutInterceptor.java, not this file. Commenting here as this is the only changed segment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spock-core/src/main/java/spock/util/concurrent/PollingConditions.java` around
lines 201 - 203, The code mixes locale-sensitive numeric formatting: some
numeric formats use Locale.ROOT (e.g., the String.format call that formats
elapsedTime/attempts) while other printf/String.format usages still rely on the
default locale when formatting "%1.2f". Locate the printf/String.format calls
that format numbers with "%1.2f" (the ones that reference
elapsedTime/attempts-style values) and change them to use Locale.ROOT (e.g.,
String.format(Locale.ROOT, ...) or printf(Locale.ROOT, ...)) so all numeric
formatting in PollingConditions uses Locale.ROOT consistently.
Vampire
force-pushed
the
vampire/fix-number-rendering
branch
from
f03bda4 to
50f48e6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@spock-core/src/main/java/org/spockframework/runtime/extension/builtin/TimeoutInterceptor.java`:
- Line 139: In TimeoutInterceptor.java (class TimeoutInterceptor), the format
string used when logging the timeout message includes both a literal "\n" and
"%n", causing double newlines; update the format string in the logging/format
call (the string starting with "[spock.lang.Timeout] Method '%s'...") to use a
single platform-specific line separator (prefer %n) and remove the redundant
"\n" so the message ends with only "%n".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3807fb9a-fe8a-4524-b5b7-e599d74fea64
📒 Files selected for processing (30)
spock-core/src/main/java/org/spockframework/compiler/ErrorReporter.javaspock-core/src/main/java/org/spockframework/compiler/InvalidSpecCompileException.javaspock-core/src/main/java/org/spockframework/compiler/SpecRewriter.javaspock-core/src/main/java/org/spockframework/compiler/WhereBlockRewriter.javaspock-core/src/main/java/org/spockframework/mock/runtime/ByteBuddyMockFactory.javaspock-core/src/main/java/org/spockframework/mock/runtime/MockInteraction.javaspock-core/src/main/java/org/spockframework/runtime/DataVariablesIterationNameProvider.javaspock-core/src/main/java/org/spockframework/runtime/FailedSetEqualityComparisonRenderer.javaspock-core/src/main/java/org/spockframework/runtime/FailedStringComparisonRenderer.javaspock-core/src/main/java/org/spockframework/runtime/InvalidSpecException.javaspock-core/src/main/java/org/spockframework/runtime/SafeIterationNameProvider.javaspock-core/src/main/java/org/spockframework/runtime/SpockExecutionException.javaspock-core/src/main/java/org/spockframework/runtime/SpockRuntime.javaspock-core/src/main/java/org/spockframework/runtime/extension/ExtensionException.javaspock-core/src/main/java/org/spockframework/runtime/extension/builtin/TimeoutInterceptor.javaspock-core/src/main/java/org/spockframework/runtime/model/TextPosition.javaspock-core/src/main/java/org/spockframework/util/Assert.javaspock-core/src/main/java/org/spockframework/util/InternalSpockError.javaspock-core/src/main/java/org/spockframework/util/IoUtil.javaspock-core/src/main/java/org/spockframework/util/VersionNumber.javaspock-core/src/main/java/spock/config/ConfigurationException.javaspock-core/src/main/java/spock/lang/Snapshotter.javaspock-core/src/main/java/spock/mock/AutoAttachExtension.javaspock-core/src/main/java/spock/util/concurrent/AsyncConditions.javaspock-core/src/main/java/spock/util/concurrent/BlockingVariable.javaspock-core/src/main/java/spock/util/concurrent/PollingConditions.javaspock-spring/src/main/java/org/spockframework/spring/mock/DelegatingInterceptor.javaspock-spring/src/main/java/org/spockframework/spring/mock/SpockDefinition.javaspock-spring/src/main/java/org/spockframework/spring/mock/SpockMockPostprocessor.javaspock-spring/src/main/java/org/spockframework/spring/mock/SpyDefinition.java
🚧 Files skipped from review as they are similar to previous changes (18)
- spock-core/src/main/java/org/spockframework/runtime/SpockRuntime.java
- spock-core/src/main/java/spock/lang/Snapshotter.java
- spock-core/src/main/java/org/spockframework/runtime/model/TextPosition.java
- spock-core/src/main/java/org/spockframework/util/VersionNumber.java
- spock-core/src/main/java/org/spockframework/runtime/DataVariablesIterationNameProvider.java
- spock-core/src/main/java/org/spockframework/compiler/SpecRewriter.java
- spock-spring/src/main/java/org/spockframework/spring/mock/SpockMockPostprocessor.java
- spock-core/src/main/java/org/spockframework/runtime/SafeIterationNameProvider.java
- spock-core/src/main/java/spock/config/ConfigurationException.java
- spock-core/src/main/java/org/spockframework/runtime/FailedStringComparisonRenderer.java
- spock-spring/src/main/java/org/spockframework/spring/mock/SpockDefinition.java
- spock-core/src/main/java/spock/util/concurrent/AsyncConditions.java
- spock-core/src/main/java/org/spockframework/runtime/FailedSetEqualityComparisonRenderer.java
- spock-core/src/main/java/org/spockframework/compiler/InvalidSpecCompileException.java
- spock-core/src/main/java/org/spockframework/util/Assert.java
- spock-core/src/main/java/org/spockframework/compiler/ErrorReporter.java
- spock-spring/src/main/java/org/spockframework/spring/mock/DelegatingInterceptor.java
- spock-core/src/main/java/org/spockframework/mock/runtime/ByteBuddyMockFactory.java
| private void logUnsuccessfulInterrupt(String methodName, long now, long timeoutAt, long waitMillis, int unsuccessfulAttempts) { | ||
| System.err.printf( | ||
| Locale.ROOT, | ||
| "[spock.lang.Timeout] Method '%s' has not stopped after timing out %1.2f seconds ago - interrupting. Next try in %1.2f seconds.\n%n", |
There was a problem hiding this comment.
Minor: Redundant newline characters.
The format string has both \n (literal newline) and %n (platform-specific line separator) at the end, resulting in double newlines in the output.
🔧 Suggested fix
- "[spock.lang.Timeout] Method '%s' has not stopped after timing out %1.2f seconds ago - interrupting. Next try in %1.2f seconds.\n%n",
+ "[spock.lang.Timeout] Method '%s' has not stopped after timing out %1.2f seconds ago - interrupting. Next try in %1.2f seconds.%n",📝 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.
Suggested change
| "[spock.lang.Timeout] Method '%s' has not stopped after timing out %1.2f seconds ago - interrupting. Next try in %1.2f seconds.\n%n", | |
| "[spock.lang.Timeout] Method '%s' has not stopped after timing out %1.2f seconds ago - interrupting. Next try in %1.2f seconds.%n", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@spock-core/src/main/java/org/spockframework/runtime/extension/builtin/TimeoutInterceptor.java`
at line 139, In TimeoutInterceptor.java (class TimeoutInterceptor), the format
string used when logging the timeout message includes both a literal "\n" and
"%n", causing double newlines; update the format string in the logging/format
call (the string starting with "[spock.lang.Timeout] Method '%s'...") to use a
single platform-specific line separator (prefer %n) and remove the redundant
"\n" so the message ends with only "%n".
50f48e6 to
d166ea1
Compare
This comment has been minimized.
This comment has been minimized.
Vampire
force-pushed
the
vampire/fix-number-rendering
branch
from
d166ea1 to
ccbd6f6
Compare
✅ All tests passed ✅🏷️ Commit: ccbd6f6 Learn more about TestLens at testlens.app. |
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
1 participant
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.
Summary by CodeRabbit