-
Notifications
You must be signed in to change notification settings - Fork 5.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8279185: Support for IsoFields in JapaneseDate/MinguoDate/ThaiBuddhistDate #7683
Conversation
👋 Welcome back naoto! A progress list of the required criteria for merging this PR into |
Webrevs
|
Is the public API change, adding the isIsoLike() method, necessary? It seems to me we already have the isSupported method for that purpose and plus isSupportedBy from the IsoFields. Would making sure the IsoFields are supported at the implementation level be sufficient? |
throw new DateTimeException("Resolve requires IsoChronology"); | ||
private static void ensureIsoLike(TemporalAccessor temporal) { | ||
if (!isIsoLike(temporal)) { | ||
throw new DateTimeException("Resolve requires ISO-like Chronology"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would the exception be easier to debug with if it mentioned the chronology that is not ISO-like?
} | ||
|
||
@Test | ||
public void test_Unit_isSupportedBy_ISO() { | ||
assertEquals(IsoFields.WEEK_BASED_YEARS.isSupportedBy(LocalDate.now()),true); | ||
assertEquals(IsoFields.WEEK_BASED_YEARS.isSupportedBy(ThaiBuddhistDate.now()),false); | ||
assertEquals(IsoFields.WEEK_BASED_YEARS.isSupportedBy(ThaiBuddhistDate.now()),true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically, comma (",") is followed by space.
@@ -677,6 +677,18 @@ public Period period(int years, int months, int days) { | |||
return Period.of(years, months, days); | |||
} | |||
|
|||
//----------------------------------------------------------------------- | |||
/** | |||
* {@inheritDoc} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather see the statement indicating that ISOChronology returns true; not a generic sentence.
(For each of the Chronologies).
"IsoChronology supports ISO based fields, such as DAY_OF_QUARTER and QUARTER_OF_YEAR."
* @see ThaiBuddhistChronology | ||
* @since 19 | ||
*/ | ||
default boolean isIsoLike() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a bit late for a name change...
How about the method name being: supportsIsoFields()
.
IsoLike seem pretty wishy washy.
Yes, I believe so. Without a means to tell whether the |
* @see java.time.temporal.IsoFields | ||
* @since 19 | ||
*/ | ||
default boolean supportsIsoFields() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of this name, as it is inconsistent with the rest of JSR310 API, which uses an is
prefix for booleans. I suggested isIsoLike
because the key question is whether the chronology is "like" ISO. I would also be OK with isBasedOnIso
, isDerivedFromIso
, isIsoBased
or something similar. Another risk here is limiting the method to refer only to IsoFields
. While that is the use case here, it isn't the case that the only fields that might be affected are in IsoFields
. Third parties may have their own fields that are suitable for use with an ISO-like chronology.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I propose isIsoBased()
for the name, which I initially thought of. If there is no objection, I will modify the spec/impl.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is IsoBased
is fine with me. "isISOLike" is too vague.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That matches the javadoc as well, that it "supports ISO based fields".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed the new method to isIsoBased()
. Modified the CSR accordingly.
* @since 19 | ||
*/ | ||
@Override | ||
public boolean isIsoBased() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this meant to be 'default'? The CSR indicated adding default methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default
method has been added to java.time.chrono.Chronology
interface. This is its overridden method implemented in IsoChronology
concrete class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@naotoj This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
Looks good to me. For the name, another option might be IsoCompatible instead of IsoBased as historically those other calendars were established before the ISO standard, although technically, in the Java language, it could be said the xChronology is ISO based. |
* @see IsoFields | ||
* @since 19 | ||
*/ | ||
default boolean isIsoBased() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the description be more specific. Each of the chronologies mentioned
say they have the same number of months, the number of days in each month, and day-of-year and leap years are the same as ISO. The week-based fields in IsoFields also depend ISO defined concepts.
Perhaps it should say that this method should return true only if all of the fields of IsoFields are supported for the chronology.
The chronology names could be links and omit the @see tags, including ISOChronology.
* @since 19 | ||
*/ | ||
@Override | ||
public boolean isIsoBased() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private static void ensureIsoBased(TemporalAccessor temporal) { | ||
if (!isIsoBased(temporal)) { | ||
throw new DateTimeException("Resolve requires ISO based chronology: " + | ||
Chronology.from(temporal)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name change doesn't add much, I'd leave both ensureIso
and isIso
method names unchanged.
static boolean isIso(TemporalAccessor temporal) { | ||
return Chronology.from(temporal).equals(IsoChronology.INSTANCE); | ||
static boolean isIsoBased(TemporalAccessor temporal) { | ||
return Chronology.from(temporal).isIsoBased(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this method be private static?
Also, move the ensureIso
method to be next to isIso
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks
@naotoj This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration! |
@naotoj This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 81 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
/integrate |
Going to push as commit ef7a9f8.
Your commit was automatically rebased without conflicts. |
Supporting
IsoFields
temporal fields in chronologies that are similar to ISO chronology. Corresponding CSR has also been drafted.Progress
Issues
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/7683/head:pull/7683
$ git checkout pull/7683
Update a local copy of the PR:
$ git checkout pull/7683
$ git pull https://git.openjdk.java.net/jdk pull/7683/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 7683
View PR using the GUI difftool:
$ git pr show -t 7683
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/7683.diff