Fix implementations of query() #376
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 explicit handling of specific TemporalQueries in multiple temporal types (precision(), chronology(), localDate(), localTime(), offset(), zone()), standardizes fallback to the parent Temporal/TemporalAccessor query, updates Javadoc, adjusts tests, and adds a public query(...) method to YearHalf. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant T as TemporalAccessor Type
note right of T #F7FBFF: Examples: DayOfMonth, DayOfYear,\nYearWeek, Quarter, YearHalf, AmPm, Half, HourMinute, OffsetDate
Client->>T: query(query)
alt query == localDate (OffsetDate)
T-->>Client: LocalDate
else query == localTime (HourMinute / OffsetDate -> null)
T-->>Client: LocalTime or null
else query == chronology()
T-->>Client: IsoChronology.INSTANCE
else query == precision()
note right of T #EFF8EB: Returned units vary:\nDAYS, WEEKS, QUARTER_YEARS,\nHALF_YEARS, HALF_DAYS, MINUTES
T-->>Client: ChronoUnit / TemporalUnit
else query == offset() or zone() (OffsetDate)
T-->>Client: ZoneOffset / ZoneId
else
T->>T: delegate to parent interface\n(TemporalAccessor.super.query / Temporal.super.query)
T-->>Client: delegated result (may be null)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 (14)
🚧 Files skipped from review as they are similar to previous changes (10)
⏰ 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)
🔇 Additional comments (15)
✨ 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/main/java/org/threeten/extra/DayOfYear.java (1)
354-356: Typo: missing space in Javadoc ("day-of-yearand")Minor editorial fix to improve readability.
Apply this diff:
- * This method checks whether this day-of-yearand the input year form + * This method checks whether this day-of-year and the input year formsrc/main/java/org/threeten/extra/HourMinute.java (1)
405-408: Punctuation nit: stray comma before periodMinor grammar fix in Javadoc.
Apply this diff:
- * The {@link #isSupported(TemporalField) supported fields} will return valid - * values based on this hour-minute,. + * The {@link #isSupported(TemporalField) supported fields} will return valid + * values based on this hour-minute.src/test/java/org/threeten/extra/TestOffsetDate.java (1)
527-531: Prefer assertNull over assertEquals(null, …) for consistency.The file already uses assertNull elsewhere (e.g., zoneId tests). Aligning here improves readability.
Apply this diff:
- assertEquals(null, TEST_2007_07_15_PONE.query(TemporalQueries.localTime())); - assertEquals(null, TemporalQueries.localTime().queryFrom(TEST_2007_07_15_PONE)); + assertNull(TEST_2007_07_15_PONE.query(TemporalQueries.localTime())); + assertNull(TemporalQueries.localTime().queryFrom(TEST_2007_07_15_PONE));
📜 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 (14)
src/main/java/org/threeten/extra/AmPm.java(1 hunks)src/main/java/org/threeten/extra/DayOfMonth.java(3 hunks)src/main/java/org/threeten/extra/DayOfYear.java(3 hunks)src/main/java/org/threeten/extra/Half.java(1 hunks)src/main/java/org/threeten/extra/HourMinute.java(2 hunks)src/main/java/org/threeten/extra/OffsetDate.java(2 hunks)src/main/java/org/threeten/extra/Quarter.java(1 hunks)src/main/java/org/threeten/extra/YearHalf.java(1 hunks)src/main/java/org/threeten/extra/YearQuarter.java(1 hunks)src/main/java/org/threeten/extra/YearWeek.java(2 hunks)src/test/java/org/threeten/extra/TestDayOfMonth.java(1 hunks)src/test/java/org/threeten/extra/TestDayOfYear.java(1 hunks)src/test/java/org/threeten/extra/TestOffsetDate.java(1 hunks)src/test/java/org/threeten/extra/TestYearWeek.java(1 hunks)
⏰ 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). (2)
- GitHub Check: build (11)
- GitHub Check: build (21)
🔇 Additional comments (21)
src/main/java/org/threeten/extra/AmPm.java (1)
318-321: Docs correctly call out direct support for precision()Javadoc now matches behavior: query(TemporalQueries.precision()) returns HALF_DAYS, and other queries delegate to the default implementation. Looks good.
src/main/java/org/threeten/extra/DayOfYear.java (3)
45-45: Import added for ChronoUnit is appropriateNeeded for returning ChronoUnit.DAYS from query(precision()). No issues.
373-376: Javadoc alignment with behavior for chronology() and precision()Documentation now states direct support for chronology() and precision(), delegating everything else via the default query. Matches intended semantics.
387-393: Direct precision() support returns ChronoUnit.DAYS (as intended)Implementation aligns with the PR objective and consistency rules. Delegation for other queries via TemporalAccessor.default is correct.
src/test/java/org/threeten/extra/TestDayOfYear.java (1)
409-409: Test expectation updated to ChronoUnit.DAYSThis matches DayOfYear.query(precision()) returning DAYS. Good coverage.
src/main/java/org/threeten/extra/HourMinute.java (2)
676-686: Javadoc for plus(..., unit) correctly states HourMinute is returnedThe updated wording for MINUTES/HOURS/HALF_DAYS matches the actual return type and implementation.
850-853: Javadoc explicitly documents direct support for localTime() and precision()Matches the implementation (returns toLocalTime() and MINUTES). All other queries correctly delegate.
src/main/java/org/threeten/extra/YearQuarter.java (1)
983-986: Javadoc explicitly mentions chronology() and precision() direct supportConsistent with the existing implementation returning IsoChronology and QUARTER_YEARS. No changes needed.
src/main/java/org/threeten/extra/Quarter.java (1)
437-440: Javadoc clearly documents direct support for chronology() and precision(). LGTM.Accurately reflects the existing implementation returning IsoChronology and QUARTER_YEARS; other queries delegate to TemporalAccessor.super.query(query).
src/main/java/org/threeten/extra/DayOfMonth.java (3)
47-47: Required ChronoUnit import is correctly added.Needed for returning ChronoUnit.DAYS from query(precision()).
375-378: Javadoc update is correct and consistent with behavior.States direct support for chronology() and precision(), with all other queries delegated.
391-393: Returning ChronoUnit.DAYS for TemporalQueries.precision() is correct.Matches the “consistent” type contract in this PR; other queries remain delegated.
src/test/java/org/threeten/extra/TestDayOfMonth.java (1)
522-530: Test expectation updated to ChronoUnit.DAYS is correct.The assertion now matches DayOfMonth.query(precision()) behavior.
src/main/java/org/threeten/extra/Half.java (1)
393-396: Javadoc alignment looks good.Documents direct support for chronology() and precision(), consistent with the existing implementation returning IsoChronology and HALF_YEARS.
src/main/java/org/threeten/extra/YearHalf.java (1)
982-985: Javadoc clearly documents supported queries.Direct support for chronology() and precision() is stated; implementation already returns IsoChronology and HALF_YEARS.
src/main/java/org/threeten/extra/OffsetDate.java (2)
1058-1061: Javadoc correctly documents direct query support.The added note about supporting localDate, chronology, offset, zone, and precision is precise and consistent with the implementation and tests. Good improvement in discoverability.
1073-1082: query(): Direct handling for common TemporalQueries is correct and matches java.time semantics.
- Returning date for localDate(), IsoChronology for chronology(), DAYS for precision(), and the ZoneOffset for both offset() and zone() is appropriate for OffsetDate.
- Not handling zoneId() (stays null) and localTime() (stays null) via fallback is correct and matches tests.
No further changes needed.
src/test/java/org/threeten/extra/TestOffsetDate.java (1)
521-525: Test coverage for localDate query is appropriate.Validates both query(...) and queryFrom(...) pathways against toLocalDate(). Looks good.
src/main/java/org/threeten/extra/YearWeek.java (2)
850-853: Javadoc updated to reflect direct support for chronology and precision.Clear and accurate. Matches the implemented behavior.
866-868: query(): precision() now returns WEEKS — correct for YearWeek.Short-circuiting for chronology and precision is appropriate; defers all other queries, maintaining expected behavior.
src/test/java/org/threeten/extra/TestYearWeek.java (1)
1063-1063: Test updated to expect ChronoUnit.WEEKS for precision.Matches the new YearWeek.query() implementation.
jodastephen
force-pushed
the
queries1
branch
2 times, most recently
from
ed53fa8 to
9f9e457
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the implementation of the query() method across multiple temporal classes to properly support precision queries, which should return appropriate ChronoUnit values instead of null for consistent temporal types.
- Updates
DayOfMonth,DayOfYear, andYearWeekto return proper precision units (DAYS and WEEKS respectively) - Enhances
OffsetDateto directly supportlocalDate()queries by returning the internal date field - Improves documentation across all affected classes to clearly specify which
TemporalQueriesare directly supported
Reviewed Changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/org/threeten/extra/DayOfMonth.java | Adds precision query support returning ChronoUnit.DAYS and updates documentation |
| src/main/java/org/threeten/extra/DayOfYear.java | Adds precision query support returning ChronoUnit.DAYS and updates documentation |
| src/main/java/org/threeten/extra/YearWeek.java | Adds precision query support returning ChronoUnit.WEEKS and updates documentation |
| src/main/java/org/threeten/extra/OffsetDate.java | Adds localDate query support and updates documentation |
| src/main/java/org/threeten/extra/HourMinute.java | Updates documentation and fixes a typo |
| src/main/java/org/threeten/extra/AmPm.java | Updates documentation to specify supported queries |
| src/main/java/org/threeten/extra/Half.java | Updates documentation to specify supported queries |
| src/main/java/org/threeten/extra/Quarter.java | Updates documentation to specify supported queries |
| src/main/java/org/threeten/extra/YearHalf.java | Updates documentation to specify supported queries |
| src/main/java/org/threeten/extra/YearQuarter.java | Updates documentation to specify supported queries |
| src/test/java/org/threeten/extra/TestDayOfMonth.java | Updates test to expect ChronoUnit.DAYS for precision query |
| src/test/java/org/threeten/extra/TestDayOfYear.java | Updates test to expect ChronoUnit.DAYS for precision query |
| src/test/java/org/threeten/extra/TestYearWeek.java | Updates test to expect ChronoUnit.WEEKS for precision query |
| src/test/java/org/threeten/extra/TestOffsetDate.java | Adds tests for localDate and localTime query behavior |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if (query == TemporalQueries.chronology()) { | ||
| return (R) IsoChronology.INSTANCE; | ||
| } else if (query == TemporalQueries.precision()) { | ||
| return (R) WEEKS; |
There was a problem hiding this comment.
The constant WEEKS is not defined in this class or imported. This should be ChronoUnit.WEEKS to match the pattern used in other classes like DayOfMonth and DayOfYear.
| return (R) WEEKS; | |
| return (R) ChronoUnit.WEEKS; |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/main/java/org/threeten/extra/YearHalf.java (2)
272-275: Fix Javadoc format token: should be H, not QThe parser and toString use H (H1/H2), but the Javadoc mentions Q. Update the format string to avoid confusing consumers.
Apply this diff:
- * The format must be {@code uuuu-'Q'Q} where the 'Q' is case insensitive. + * The format must be {@code uuuu-'H'H} where the 'H' is case insensitive.
453-456: Javadoc punctuation typoThere’s an extra comma/period sequence.
Apply this diff:
- * The {@link #isSupported(TemporalField) supported fields} will return valid - * values based on this year-half,. + * The {@link #isSupported(TemporalField) supported fields} will return valid + * values based on this year-half.src/main/java/org/threeten/extra/Quarter.java (1)
381-387: Javadoc wording nit: “month” → “quarter”Minor doc mismatch in length() Javadoc.
Apply this diff:
- * @return the length of this month in days, from 90 to 92 + * @return the length of this quarter in days, from 90 to 92
📜 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 (14)
src/main/java/org/threeten/extra/AmPm.java(1 hunks)src/main/java/org/threeten/extra/DayOfMonth.java(3 hunks)src/main/java/org/threeten/extra/DayOfYear.java(4 hunks)src/main/java/org/threeten/extra/Half.java(1 hunks)src/main/java/org/threeten/extra/HourMinute.java(3 hunks)src/main/java/org/threeten/extra/OffsetDate.java(2 hunks)src/main/java/org/threeten/extra/Quarter.java(1 hunks)src/main/java/org/threeten/extra/YearHalf.java(1 hunks)src/main/java/org/threeten/extra/YearQuarter.java(1 hunks)src/main/java/org/threeten/extra/YearWeek.java(2 hunks)src/test/java/org/threeten/extra/TestDayOfMonth.java(1 hunks)src/test/java/org/threeten/extra/TestDayOfYear.java(1 hunks)src/test/java/org/threeten/extra/TestOffsetDate.java(1 hunks)src/test/java/org/threeten/extra/TestYearWeek.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/java/org/threeten/extra/HourMinute.java
🚧 Files skipped from review as they are similar to previous changes (10)
- src/test/java/org/threeten/extra/TestOffsetDate.java
- src/test/java/org/threeten/extra/TestDayOfMonth.java
- src/test/java/org/threeten/extra/TestYearWeek.java
- src/main/java/org/threeten/extra/Half.java
- src/main/java/org/threeten/extra/OffsetDate.java
- src/main/java/org/threeten/extra/DayOfMonth.java
- src/main/java/org/threeten/extra/DayOfYear.java
- src/test/java/org/threeten/extra/TestDayOfYear.java
- src/main/java/org/threeten/extra/YearWeek.java
- src/main/java/org/threeten/extra/YearQuarter.java
⏰ 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 (3)
src/main/java/org/threeten/extra/AmPm.java (1)
313-316: Direct precision support and fallback delegation look correctReturning HALF_DAYS for TemporalQueries.precision() is semantically right for AmPm, and delegating other queries to TemporalAccessor.super.query(query) aligns with java.time patterns.
Consider adding/adjusting unit tests to assert:
- AmPm.AM.query(TemporalQueries.precision()) == ChronoUnit.HALF_DAYS
- Other common queries (localDate, localTime, zone, offset) return null as expected for this type.
Also applies to: 323-330
src/main/java/org/threeten/extra/YearHalf.java (1)
977-980: Chronology/precision handling aligns with the spec and the rest of the PR
- Returning IsoChronology.INSTANCE for TemporalQueries.chronology() is appropriate for this ISO-based type.
- Returning HALF_YEARS for TemporalQueries.precision() matches the unit semantics used elsewhere in YearHalf.
- Fallback to Temporal.super.query(query) is the right choice for a Temporal implementation.
Recommend tests to cover:
- YearHalf.of(2024, 2).query(TemporalQueries.chronology()) == IsoChronology.INSTANCE
- YearHalf.of(2024, 2).query(TemporalQueries.precision()) == HALF_YEARS
Also applies to: 987-996
src/main/java/org/threeten/extra/Quarter.java (1)
432-435: Query implementation is consistent and correct
- Returning IsoChronology.INSTANCE for chronology() and QUARTER_YEARS for precision() is correct for Quarter.
- Delegating other queries to TemporalAccessor.super.query(query) matches java.time conventions and keeps behavior consistent with similar types in this PR.
Also applies to: 442-451
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/main/java/org/threeten/extra/Half.java (2)
178-186: Typo in example: “4th half” isn’t a valid conceptThere are only two halves. Consider updating the example to avoid confusion.
Apply this small doc tweak:
- * Gets the textual representation, such as 'H1' or '4th half'. + * Gets the textual representation, such as 'H1' or '1st half'.
364-369: Javadoc wording: “month” → “half”Return description refers to “month” instead of “half”.
- * @return the length of this month in days, 181, 182 or 184 + * @return the length of this half in days, 181, 182 or 184
📜 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 (14)
src/main/java/org/threeten/extra/AmPm.java(1 hunks)src/main/java/org/threeten/extra/DayOfMonth.java(3 hunks)src/main/java/org/threeten/extra/DayOfYear.java(4 hunks)src/main/java/org/threeten/extra/Half.java(1 hunks)src/main/java/org/threeten/extra/HourMinute.java(3 hunks)src/main/java/org/threeten/extra/OffsetDate.java(2 hunks)src/main/java/org/threeten/extra/Quarter.java(2 hunks)src/main/java/org/threeten/extra/YearHalf.java(3 hunks)src/main/java/org/threeten/extra/YearQuarter.java(2 hunks)src/main/java/org/threeten/extra/YearWeek.java(2 hunks)src/test/java/org/threeten/extra/TestDayOfMonth.java(1 hunks)src/test/java/org/threeten/extra/TestDayOfYear.java(1 hunks)src/test/java/org/threeten/extra/TestOffsetDate.java(1 hunks)src/test/java/org/threeten/extra/TestYearWeek.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- src/main/java/org/threeten/extra/YearWeek.java
- src/main/java/org/threeten/extra/DayOfYear.java
- src/test/java/org/threeten/extra/TestDayOfMonth.java
- src/test/java/org/threeten/extra/TestDayOfYear.java
- src/main/java/org/threeten/extra/YearQuarter.java
- src/test/java/org/threeten/extra/TestYearWeek.java
- src/main/java/org/threeten/extra/OffsetDate.java
- src/main/java/org/threeten/extra/HourMinute.java
- src/main/java/org/threeten/extra/DayOfMonth.java
- src/main/java/org/threeten/extra/AmPm.java
- src/test/java/org/threeten/extra/TestOffsetDate.java
⏰ 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 (11)
- GitHub Check: build (21)
- GitHub Check: build (21)
- GitHub Check: build (11)
🔇 Additional comments (6)
src/main/java/org/threeten/extra/Quarter.java (2)
386-386: Javadoc fix matches actual semantics“length of this quarter in days, from 90 to 92” is correct and removes the previous mismatch. LGTM.
432-435: Query Javadoc: clear and consistent with implementationStating that chronology() and precision() are directly supported and deferring others to TemporalAccessor#query is the right contract here and aligns with the PR goal.
src/main/java/org/threeten/extra/Half.java (1)
388-391: Query Javadoc alignment looks goodDocumenting direct support for chronology() and precision() with fallback to TemporalAccessor#query is consistent with the new behavior.
src/main/java/org/threeten/extra/YearHalf.java (3)
273-273: Parse format Javadoc corrected to ‘H’The format note now correctly documents uuuu-'H'H. Looks good.
455-456: Minor Javadoc polish“values based on this year-half.” punctuation/readability fix is fine.
973-996: Adds explicit query() support for chronology and precision
- Returning IsoChronology for chronology() and HALF_YEARS for precision() is correct for a consistent temporal type.
- Delegating other queries to Temporal.super.query(query) is the right fallback since YearHalf implements Temporal (not just TemporalAccessor).
- Suppressing “unchecked” is appropriate for the cast pattern used here.
No further changes needed.
Precision query is defined to return null when date is inconsistent `MonthDay` is inconsistent, but all the clases here are consistent Fix implementations and enhance docs Fix other minor Javadoc issues
1 participant
Precision query is defined to return null when date is inconsistent.
MonthDayis inconsistent, but all the clases here are consistent.Fix implementations and enhance docs
Summary by CodeRabbit
New Features
Documentation
Tests