Fix code style issues#378
Merged
Merged
Conversation
|
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 (53)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (20)
🚧 Files skipped from review as they are similar to previous changes (25)
📝 WalkthroughWalkthroughRepository-wide, behavior-preserving edits: normalize boolean negations, simplify compareTo implementations, fix int-overflow by using long arithmetic, tidy streams/formatting, adopt method references and CharSequence.contentEquals, update generics/diamond usage, and reflow Javadoc across chronology, calendar, and scale classes. ChangesCode Quality and Standard Refactoring Across ThreeTen-Extra
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) 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 |
There was a problem hiding this comment.
Pull request overview
This PR focuses on code style cleanups across the threeten-extra codebase, primarily simplifying boolean expressions, tightening small arithmetic/boxing usages, and polishing Javadoc wording.
Changes:
- Simplified a number of
== falseboolean comparisons and lambda expressions into more idiomatic Java. - Adjusted arithmetic expressions/casts to avoid unintended overflow/boxing and improve clarity.
- Polished multiple Javadoc sentences and formatting for readability/consistency.
Reviewed changes
Copilot reviewed 53 out of 53 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/org/threeten/extra/Years.java | Simplifies compareTo implementation. |
| src/main/java/org/threeten/extra/YearQuarter.java | Javadoc wording tweaks; boolean simplification; method reference cleanup. |
| src/main/java/org/threeten/extra/YearHalf.java | Javadoc wording tweaks; boolean simplification; method reference cleanup. |
| src/main/java/org/threeten/extra/Weeks.java | Simplifies compareTo implementation. |
| src/main/java/org/threeten/extra/Temporals.java | Simplifies boolean condition in unit validation. |
| src/main/java/org/threeten/extra/TemporalFields.java | Simplifies boolean checks before throwing UnsupportedTemporalTypeException. |
| src/main/java/org/threeten/extra/Seconds.java | Simplifies compareTo implementation. |
| src/main/java/org/threeten/extra/scale/UtcRules.java | Javadoc punctuation/wording edits (and adjacent doc area touched). |
| src/main/java/org/threeten/extra/scale/UtcInstant.java | Javadoc punctuation tweak; modernized hashCode. |
| src/main/java/org/threeten/extra/scale/TimeSource.java | Javadoc wording tweak. |
| src/main/java/org/threeten/extra/scale/TaiInstant.java | Javadoc tweaks; minor arithmetic/casting cleanup; hashCode tweak. |
| src/main/java/org/threeten/extra/scale/SystemUtcRules.java | AtomicReference initialization cleanup; boolean simplifications; exception/Javadoc adjustments. |
| src/main/java/org/threeten/extra/Quarter.java | Simplifies chronology check in adjustInto. |
| src/main/java/org/threeten/extra/PeriodDuration.java | Javadoc hyphenation; simplifies ISO chronology check. |
| src/main/java/org/threeten/extra/PackedFields.java | Simplifies boolean check; fixes numeric literal types; simplifies unboxing comparison. |
| src/main/java/org/threeten/extra/OffsetDate.java | Javadoc punctuation tweak. |
| src/main/java/org/threeten/extra/Months.java | Simplifies compareTo implementation. |
| src/main/java/org/threeten/extra/Minutes.java | Simplifies compareTo implementation. |
| src/main/java/org/threeten/extra/LocalDateRange.java | Simplifies boolean checks; updates Spliterator construction (needs Java 8 compatibility fix). |
| src/main/java/org/threeten/extra/Interval.java | Simplifies boolean checks in intersection/union. |
| src/main/java/org/threeten/extra/Hours.java | Javadoc grammar fix; simplifies compareTo. |
| src/main/java/org/threeten/extra/HourMinute.java | Javadoc grammar tweaks; adds casts to avoid overflow in minute calculations. |
| src/main/java/org/threeten/extra/Half.java | Simplifies chronology check in adjustInto. |
| src/main/java/org/threeten/extra/Days.java | Simplifies compareTo implementation. |
| src/main/java/org/threeten/extra/DayOfYear.java | Simplifies ISO chronology checks. |
| src/main/java/org/threeten/extra/DayOfMonth.java | Simplifies ISO chronology checks. |
| src/main/java/org/threeten/extra/chrono/Symmetry454Date.java | Simplifies toEpochDay to a single return expression. |
| src/main/java/org/threeten/extra/chrono/Symmetry454Chronology.java | Javadoc wording/hyphenation tweaks; removes unused constant; simplifies eras(). |
| src/main/java/org/threeten/extra/chrono/Symmetry010Date.java | Minor wording tweak; simplifies condition; adds casts to avoid overflow; simplifies toEpochDay. |
| src/main/java/org/threeten/extra/chrono/Symmetry010Chronology.java | Javadoc wording/hyphenation tweaks; removes unused constants; simplifies eras(). |
| src/main/java/org/threeten/extra/chrono/PaxDate.java | Javadoc hyphenation tweaks; simplifies arithmetic and lengthOfMonth logic. |
| src/main/java/org/threeten/extra/chrono/PaxChronology.java | Javadoc wording tweak; simplifies leap year predicate; simplifies eras(). |
| src/main/java/org/threeten/extra/chrono/JulianDate.java | Javadoc hyphenation; simplifies leap checks; minor casting cleanup. |
| src/main/java/org/threeten/extra/chrono/JulianChronology.java | Simplifies instanceof check; simplifies eras(). |
| src/main/java/org/threeten/extra/chrono/InternationalFixedDate.java | Javadoc hyphenation tweak; punctuation tweak in comment. |
| src/main/java/org/threeten/extra/chrono/InternationalFixedChronology.java | Javadoc hyphenation/grammar tweaks; simplifies eras(). |
| src/main/java/org/threeten/extra/chrono/EthiopicDate.java | Simplifies leap year check. |
| src/main/java/org/threeten/extra/chrono/EthiopicChronology.java | Javadoc grammar tweaks; simplifies instanceof check; simplifies eras(). |
| src/main/java/org/threeten/extra/chrono/DiscordianEra.java | Simplifies of(int) logic. |
| src/main/java/org/threeten/extra/chrono/DiscordianDate.java | Simplifies control flow; adds casts to avoid overflow; minor string builder append simplification. |
| src/main/java/org/threeten/extra/chrono/DiscordianChronology.java | Simplifies eras(). |
| src/main/java/org/threeten/extra/chrono/CopticDate.java | Simplifies leap year check. |
| src/main/java/org/threeten/extra/chrono/CopticChronology.java | Simplifies instanceof check; simplifies eras(). |
| src/main/java/org/threeten/extra/chrono/BritishCutoverDate.java | Simplifies leap year check in range computation. |
| src/main/java/org/threeten/extra/chrono/BritishCutoverChronology.java | Simplifies instanceof check; simplifies eras(). |
| src/main/java/org/threeten/extra/chrono/AccountingYearDivision.java | Simplifies arithmetic expression for binary-search insert position. |
| src/main/java/org/threeten/extra/chrono/AccountingDate.java | Simplifies constant expression; removes unnecessary cast in floorMod. |
| src/main/java/org/threeten/extra/chrono/AccountingChronologyBuilder.java | Fixes punctuation in Javadoc. |
| src/main/java/org/threeten/extra/chrono/AccountingChronology.java | Javadoc wording tweak; minor arithmetic literal type fixes; variable rename cleanup. |
| src/main/java/org/threeten/extra/chrono/AbstractNileDate.java | Removes unnecessary cast to long. |
| src/main/java/org/threeten/extra/chrono/AbstractNileChronology.java | Removes unused constants. |
| src/main/java/org/threeten/extra/chrono/AbstractDate.java | Simplifies arithmetic/casts to avoid overflow; minor floorMod and constant type cleanups. |
| src/main/java/org/threeten/extra/AmountFormats.java | Removes redundant .sequential(); minor boolean simplification; loop refactor; safer string comparison. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/threeten/extra/chrono/DiscordianDate.java (1)
621-627:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix lossy long→int assignments from
Math.floorModinplusMonths/plusWeeks
calcEmislong, soMath.floorMod(calcEm, ...)selects thelongoverload and returnslong; assigning it tointcauses compilation failures at thenewMonthandnewDayOfYearlines. Reintroduce the casts.Suggested fix
DiscordianDate plusMonths(long months) { if (months == 0) { return this; } long calcEm = Math.addExact(getProlepticMonth(), months); int newYear = Math.toIntExact(Math.floorDiv(calcEm, MONTHS_IN_YEAR)); - int newMonth = Math.floorMod(calcEm, MONTHS_IN_YEAR) + 1; + int newMonth = (int) Math.floorMod(calcEm, MONTHS_IN_YEAR) + 1; // If the starting date was St. Tib's Day, attempt to retain that status. if (month == 0 && newMonth == 1) { newMonth = 0; } return resolvePrevious(newYear, newMonth, day); @@ DiscordianDate plusWeeks(long weeks) { if (weeks == 0) { return this; } long calcEm = Math.addExact(getProlepticWeek(), weeks); int newYear = Math.toIntExact(Math.floorDiv(calcEm, WEEKS_IN_YEAR)); // Give St. Tib's Day the same day-of-week as the day after it. - int newDayOfYear = Math.floorMod(calcEm, WEEKS_IN_YEAR) * DAYS_IN_WEEK + (month == 0 ? DAYS_IN_WEEK : get(DAY_OF_WEEK)); + int newDayOfYear = (int) Math.floorMod(calcEm, WEEKS_IN_YEAR) * DAYS_IN_WEEK + (month == 0 ? DAYS_IN_WEEK : get(DAY_OF_WEEK)); // Need to offset day-of-year if leap year, and not heading to St. Tib's Day again. if (DiscordianChronology.INSTANCE.isLeapYear(newYear) && (newDayOfYear > ST_TIBS_OFFSET || (newDayOfYear == ST_TIBS_OFFSET && month != 0))) { newDayOfYear++; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/org/threeten/extra/chrono/DiscordianDate.java` around lines 621 - 627, The assignments in plusMonths and plusWeeks use Math.floorMod(calcEm, ...) where calcEm is long, so the long overload returns a long being assigned to int (newMonth, newDayOfYear) causing compilation errors; fix by casting the Math.floorMod(...) results to int (e.g. int newMonth = (int) Math.floorMod(calcEm, MONTHS_IN_YEAR) + 1) and similarly cast the floorMod result used to compute newDayOfYear in plusWeeks so the int variables receive (int) values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/org/threeten/extra/chrono/AbstractDate.java`:
- Around line 175-176: The Math.floorMod(long,long) calls return long but the
code expects ints; modify getDayOfWeek() and plusMonths(long) to cast the
floorMod results to int before further arithmetic—e.g., in getDayOfWeek() wrap
Math.floorMod(toEpochDay() + 3, 7) with (int) and in plusMonths(long) cast the
result of Math.floorMod(calcEm, lengthOfYearInMonths()) to (int) when assigning
newMonth (and any similar uses), keeping the +1 arithmetic on ints.
In `@src/main/java/org/threeten/extra/chrono/PaxDate.java`:
- Line 288: The variable dayOfCycle is declared as int but
Math.floorMod(paxEpochDay - ((long) longCycle) * DAYS_PER_LONG_CYCLE,
DAYS_PER_CYCLE) returns a long, causing a lossy conversion; update the
assignment in PaxDate so the long result is converted explicitly to int (e.g.,
cast the Math.floorMod result to int or use Math.toIntExact) referencing the
dayOfCycle variable, paxEpochDay, longCycle, DAYS_PER_LONG_CYCLE and
DAYS_PER_CYCLE to ensure the value fits into an int.
In `@src/main/java/org/threeten/extra/LocalDateRange.java`:
- Line 605: The anonymous Spliterator instantiation in LocalDateRange (the
spliterator variable created with new Spliterators.AbstractSpliterator<>(...))
uses the diamond operator which breaks Java 8; change the anonymous class
instantiation to use an explicit generic type argument (e.g. new
Spliterators.AbstractSpliterator<LocalDate>(...)) so the compiler can infer the
type without the diamond in the anonymous class for the LocalDateRange
spliterator.
In `@src/main/java/org/threeten/extra/scale/TaiInstant.java`:
- Around line 137-140: In ofTaiSeconds(long taiSeconds, long nanoAdjustment)
restore the explicit narrowing conversion for nos: Math.floorMod(nanoAdjustment,
NANOS_PER_SECOND) returns a long so cast the result to int (e.g., int nos =
(int) Math.floorMod(...)) to make the assignment compile safely; update the nos
initialization in the ofTaiSeconds method accordingly and keep the existing
comment about the cast being safe.
In `@src/main/java/org/threeten/extra/scale/UtcRules.java`:
- Around line 48-50: Fix the Javadoc typo in the UtcRules class comment: remove
the extra word "in" so the sentence reads "place it on the classpath" instead of
"place it in on the classpath". Edit the class-level Javadoc in UtcRules to
update that sentence accordingly (look for the comment block at the top of the
UtcRules class).
---
Outside diff comments:
In `@src/main/java/org/threeten/extra/chrono/DiscordianDate.java`:
- Around line 621-627: The assignments in plusMonths and plusWeeks use
Math.floorMod(calcEm, ...) where calcEm is long, so the long overload returns a
long being assigned to int (newMonth, newDayOfYear) causing compilation errors;
fix by casting the Math.floorMod(...) results to int (e.g. int newMonth = (int)
Math.floorMod(calcEm, MONTHS_IN_YEAR) + 1) and similarly cast the floorMod
result used to compute newDayOfYear in plusWeeks so the int variables receive
(int) values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cb79e4bb-c190-4235-a573-b5aebc2a8b03
📒 Files selected for processing (53)
src/main/java/org/threeten/extra/AmountFormats.javasrc/main/java/org/threeten/extra/DayOfMonth.javasrc/main/java/org/threeten/extra/DayOfYear.javasrc/main/java/org/threeten/extra/Days.javasrc/main/java/org/threeten/extra/Half.javasrc/main/java/org/threeten/extra/HourMinute.javasrc/main/java/org/threeten/extra/Hours.javasrc/main/java/org/threeten/extra/Interval.javasrc/main/java/org/threeten/extra/LocalDateRange.javasrc/main/java/org/threeten/extra/Minutes.javasrc/main/java/org/threeten/extra/Months.javasrc/main/java/org/threeten/extra/OffsetDate.javasrc/main/java/org/threeten/extra/PackedFields.javasrc/main/java/org/threeten/extra/PeriodDuration.javasrc/main/java/org/threeten/extra/Quarter.javasrc/main/java/org/threeten/extra/Seconds.javasrc/main/java/org/threeten/extra/TemporalFields.javasrc/main/java/org/threeten/extra/Temporals.javasrc/main/java/org/threeten/extra/Weeks.javasrc/main/java/org/threeten/extra/YearHalf.javasrc/main/java/org/threeten/extra/YearQuarter.javasrc/main/java/org/threeten/extra/Years.javasrc/main/java/org/threeten/extra/chrono/AbstractDate.javasrc/main/java/org/threeten/extra/chrono/AbstractNileChronology.javasrc/main/java/org/threeten/extra/chrono/AbstractNileDate.javasrc/main/java/org/threeten/extra/chrono/AccountingChronology.javasrc/main/java/org/threeten/extra/chrono/AccountingChronologyBuilder.javasrc/main/java/org/threeten/extra/chrono/AccountingDate.javasrc/main/java/org/threeten/extra/chrono/AccountingYearDivision.javasrc/main/java/org/threeten/extra/chrono/BritishCutoverChronology.javasrc/main/java/org/threeten/extra/chrono/BritishCutoverDate.javasrc/main/java/org/threeten/extra/chrono/CopticChronology.javasrc/main/java/org/threeten/extra/chrono/CopticDate.javasrc/main/java/org/threeten/extra/chrono/DiscordianChronology.javasrc/main/java/org/threeten/extra/chrono/DiscordianDate.javasrc/main/java/org/threeten/extra/chrono/DiscordianEra.javasrc/main/java/org/threeten/extra/chrono/EthiopicChronology.javasrc/main/java/org/threeten/extra/chrono/EthiopicDate.javasrc/main/java/org/threeten/extra/chrono/InternationalFixedChronology.javasrc/main/java/org/threeten/extra/chrono/InternationalFixedDate.javasrc/main/java/org/threeten/extra/chrono/JulianChronology.javasrc/main/java/org/threeten/extra/chrono/JulianDate.javasrc/main/java/org/threeten/extra/chrono/PaxChronology.javasrc/main/java/org/threeten/extra/chrono/PaxDate.javasrc/main/java/org/threeten/extra/chrono/Symmetry010Chronology.javasrc/main/java/org/threeten/extra/chrono/Symmetry010Date.javasrc/main/java/org/threeten/extra/chrono/Symmetry454Chronology.javasrc/main/java/org/threeten/extra/chrono/Symmetry454Date.javasrc/main/java/org/threeten/extra/scale/SystemUtcRules.javasrc/main/java/org/threeten/extra/scale/TaiInstant.javasrc/main/java/org/threeten/extra/scale/TimeSource.javasrc/main/java/org/threeten/extra/scale/UtcInstant.javasrc/main/java/org/threeten/extra/scale/UtcRules.java
💤 Files with no reviewable changes (1)
- src/main/java/org/threeten/extra/chrono/AbstractNileChronology.java
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
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.
Fix a bunch of code style issues.
Summary by CodeRabbit
Bug Fixes
Documentation
Refactor
Style