Add PeriodDuration.truncatedTo#374
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds PeriodDuration.truncatedTo(TemporalUnit) that returns a new PeriodDuration with only the duration component truncated to the specified temporal unit; validates unit, ensures unit divides a standard day where required, and leaves the period unchanged. Tests added for various units and invalid unit behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant PeriodDuration
participant TemporalUnit
Caller->>PeriodDuration: truncatedTo(unit)
activate PeriodDuration
PeriodDuration->>TemporalUnit: unit.getDuration()
TemporalUnit-->>PeriodDuration: unitDuration (seconds, nanos)
Note right of PeriodDuration: validate non-null, unitDuration <= SECONDS_PER_DAY\nand NANOS_PER_DAY % unitNanos == 0
PeriodDuration->>PeriodDuration: compute nod = duration.toNanos() mod NANOS_PER_DAY
PeriodDuration->>PeriodDuration: result = floor(nod / unitNanos) * unitNanos
PeriodDuration->>PeriodDuration: adjusted = duration.plusNanos(result - nod)
PeriodDuration-->>Caller: return new PeriodDuration(period, adjusted)
deactivate PeriodDuration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/main/java/org/threeten/extra/PeriodDuration.java(1 hunks)src/test/java/org/threeten/extra/TestPeriodDuration.java(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/test/java/org/threeten/extra/TestPeriodDuration.java (1)
src/main/java/org/threeten/extra/PeriodDuration.java (1)
PeriodDuration(84-699)
🪛 GitHub Actions: Build
src/main/java/org/threeten/extra/PeriodDuration.java
[error] 645-645: Maven build failed during 'mvn install site' due to compilation error: cannot find symbol: method truncatedTo(TemporalUnit) on variable duration of type Duration (PeriodDuration.java:645). This method is not available in Java 8; upgrade to Java 9+ or replace with an alternative truncation approach.
🔇 Additional comments (2)
src/test/java/org/threeten/extra/TestPeriodDuration.java (2)
36-36: LGTM - Clean static import addition.The addition of the MINUTES static import is appropriate for the new test method and follows the existing import pattern in the test file.
384-417: Comprehensive test coverage for truncation behavior.The test method thoroughly validates the truncation functionality for both MINUTES and DAYS units. The test cases correctly verify that:
- The period component (P1Y2M3D) remains unchanged in all scenarios
- Duration truncation works correctly for edge cases (0s, 59s, 60s boundary)
- DAYS truncation handles sub-day and multi-day scenarios properly
The test logic aligns perfectly with the expected truncation behavior described in the method's Javadoc.
82ef232 to
5fe11e7
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new truncatedTo method to the PeriodDuration class that allows truncating the duration portion to a specified temporal unit while preserving the period portion. This addresses issue #269 by providing functionality similar to Duration.truncatedTo() but adapted for the combined period-duration structure.
- Adds
truncatedTo(TemporalUnit)method that truncates only the duration part - Includes validation to ensure the unit divides evenly into a standard day
- Maintains immutability by returning a new instance
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/main/java/org/threeten/extra/PeriodDuration.java | Implements the truncatedTo method with proper validation and nanosecond constants |
| src/test/java/org/threeten/extra/TestPeriodDuration.java | Adds comprehensive test coverage for truncation behavior and error cases |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
| } | ||
| Duration unitDuration = unit.getDuration(); | ||
| if (unitDuration.getSeconds() > SECONDS_PER_DAY) { | ||
| throw new UnsupportedTemporalTypeException("Unit is too large to be used for truncation"); |
There was a problem hiding this comment.
The error message should be more specific about what constitutes 'too large'. Consider: 'Unit duration exceeds one day and cannot be used for truncation'
| throw new UnsupportedTemporalTypeException("Unit is too large to be used for truncation"); | |
| throw new UnsupportedTemporalTypeException("Unit duration exceeds one day and cannot be used for truncation"); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/test/java/org/threeten/extra/TestPeriodDuration.java (1)
384-425: Good coverage of truncation basics; consider adding edge cases (negatives, nanos, identity, null unit).The tests validate NANOS/SECONDS/MINUTES/DAYS and an invalid MONTHS unit. To harden behavior and guard regressions, add:
- Negative durations (truncate toward zero semantics).
- Nanos-to-seconds truncation (drop sub-second nanos).
- Identity for NANOS (method promises to return this).
- Null unit throws NPE.
Proposed additions within this test method:
@Test public void test_truncatedTo() { + // identity when truncating to NANOS + PeriodDuration base = PeriodDuration.of(P1Y2M3D, DUR_5); + assertTrue(base == base.truncatedTo(NANOS)); + assertEquals( PeriodDuration.of(P1Y2M3D, DUR_5), PeriodDuration.of(P1Y2M3D, DUR_5).truncatedTo(NANOS)); assertEquals( PeriodDuration.of(P1Y2M3D, DUR_5), PeriodDuration.of(P1Y2M3D, DUR_5).truncatedTo(SECONDS)); + // drop nanos when truncating to SECONDS + assertEquals( + PeriodDuration.of(P1Y2M3D, Duration.ofSeconds(5)), + PeriodDuration.of(P1Y2M3D, Duration.ofSeconds(5, 999_999_999)).truncatedTo(SECONDS)); @@ assertEquals( PeriodDuration.of(P1Y2M3D, Duration.ofMinutes(2)), PeriodDuration.of(P1Y2M3D, Duration.ofSeconds(120)).truncatedTo(MINUTES)); + // multi-day duration: only within-day portion is truncated; day-sized part remains + assertEquals( + PeriodDuration.of(P1Y2M3D, Duration.ofDays(2).plusMinutes(1)), + PeriodDuration.of(P1Y2M3D, Duration.ofDays(2).plusSeconds(61)).truncatedTo(MINUTES)); + // negatives: truncate toward zero + assertEquals( + PeriodDuration.of(P1Y2M3D, Duration.ZERO), + PeriodDuration.of(P1Y2M3D, Duration.ofSeconds(-59)).truncatedTo(MINUTES)); + assertEquals( + PeriodDuration.of(P1Y2M3D, Duration.ofMinutes(-1)), + PeriodDuration.of(P1Y2M3D, Duration.ofSeconds(-61)).truncatedTo(MINUTES)); + assertEquals( + PeriodDuration.of(P1Y2M3D, Duration.ofDays(-2)), + PeriodDuration.of(P1Y2M3D, Duration.ofHours(-51)).truncatedTo(DAYS)); @@ assertThrows(DateTimeException.class, () -> PeriodDuration.of(P1Y2M3D, Duration.ofHours(51)).truncatedTo(MONTHS)); + // null unit + assertThrows(NullPointerException.class, () -> PeriodDuration.of(P1Y2M3D, DUR_5).truncatedTo(null)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/main/java/org/threeten/extra/PeriodDuration.java(2 hunks)src/test/java/org/threeten/extra/TestPeriodDuration.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/threeten/extra/PeriodDuration.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/test/java/org/threeten/extra/TestPeriodDuration.java (1)
src/main/java/org/threeten/extra/PeriodDuration.java (1)
PeriodDuration(84-723)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build (21)
- GitHub Check: build (11)
- GitHub Check: build (11)
- GitHub Check: build (21)
🔇 Additional comments (2)
src/test/java/org/threeten/extra/TestPeriodDuration.java (2)
36-36: Static import for MINUTES is appropriate and consistent with usage.This enables concise references to MINUTES in the new truncation tests.
384-425: Tests are clear and correctly assert that only the duration part changes.
- Asserts keep the period as P1Y2M3D across all cases.
- Boundary checks for minutes and days are spot-on, including exact multiples and just-below thresholds.
- Exception case for MONTHS is appropriate via DateTimeException.
No blocking issues from my side.
Only truncate the duration part Fixes #269
5fe11e7 to
4c4e6e7
Compare
Only truncate the duration part
Fixes #269
Summary by CodeRabbit
New Features
Tests