-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add support for "week of month" in SimpleDateTimeFormatter #11103
base: main
Are you sure you want to change the base?
Add support for "week of month" in SimpleDateTimeFormatter #11103
Conversation
✅ Deploy Preview for meta-velox canceled.
|
4a95917
to
a3205d0
Compare
@rui-mo Could you help to review this PR please? Thanks. |
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.
Thanks. Looks good to me overall.
bool centuryFormat = false; | ||
|
||
bool isYearOfEra = false; // Year of era cannot be zero or negative. | ||
bool hasYear = false; // Whether year was explicitly specified. | ||
bool hasYear = false; |
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.
nit: is the comment Whether year was explicitly specified.
removed by accident?
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.
Thanks for catching that. Updated.
bool hasYear = false; // Whether year was explicitly specified. | ||
bool hasYear = false; | ||
// Whether dayOfWeek was explicitly specified. | ||
bool hasDayOfWeek = false; |
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.
nit: perhaps follow the style of other comments to add comment after the code.
if (!date.weekOfMonthDateFormat) { | ||
date.weekDateFormat = true; | ||
} | ||
date.hasDayOfWeek = 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.
nit: perhaps put it near L1001.
@rui-mo I've updated code based on the feedback. Could you review it again? Thanks. |
java.text.SimpleDateFormat
supports using 'week of month' to parse/format date. The specifier of 'week of month' is 'W'. Now DateTimeFormatter supports 3 group of fields specifying the day within the year. They are following combinations:This PR introduces a new combination that is
year + month + weekOfMonth + dayOfWeek
and adds support for "week of month" in SimpleDateTimeFormatter.Relates issue : #10354