Added ISOWeek to System.Globalization #18456
Conversation
|
@khellang thanks for submitting the PR. I'll try to take a look soon. meanwhile, could you please try to submit a PR in corefx for adding tests to the introduced APIs? I know we'll not be able to merge the corefx PR before merging this coreclr PR and getting the updated coreclr package to corefx. but it will be nice to have this ready and run the tests against your private bits. https://github.com/dotnet/corefx/blob/8e842fa29e14694cca96c6e39a38199c55a3a02e/Documentation/project-docs/developer-guide.md#testing-with-private-coreclr-bits |
| private const long MaxMillis = (long)DaysTo10000 * MillisPerDay; | ||
|
|
||
| internal const int MinYear = 1; | ||
| internal const int MaxYear = 9999; |
There was a problem hiding this comment.
internal const int MaxYear = 9999; [](start = 7, length = 35)
This is already defined in https://github.com/dotnet/coreclr/blob/master/src/System.Private.CoreLib/shared/System/Globalization/GregorianCalendar.cs#L26
I would suggest to either define MinYear there too or let the Gregorian calendar use the ones defined here. I mean at the end we need to have these constants defined in one place
| return ToDateTime(year, GetWeeksInYear(year), DayOfWeek.Sunday); | ||
| } | ||
|
|
||
| // From https://en.wikipedia.org/wiki/ISO_week_date#Weeks_per_year. |
There was a problem hiding this comment.
From https://en.wikipedia.org/wiki/ISO_week_date#Weeks_per_year. [](start = 11, length = 64)
would be good to add the comment:
The long years, with 53 weeks in them, can be described by any of the following equivalent definitions:
any year starting on Thursday (dominical letter D or DC) and any leap year starting on Wednesday (ED)
any year ending on Thursday (D, ED) and any leap year ending on Friday (DC)
years in which 1 January and 31 December (in common years) or either (in leap years) are Thursdays
All other week-numbering years are short years and have 52 weeks.
|
@tarekgh Thanks for being so quick to review this!
I will. Have a set of tests ready to go 😊 |
| throw new ArgumentOutOfRangeException(nameof(week), SR.ArgumentOutOfRange_Week_ISO); | ||
| } | ||
|
|
||
| if ((int)dayOfWeek < 0 || (int)dayOfWeek > 6) |
There was a problem hiding this comment.
if ((int)dayOfWeek < 0 || (int)dayOfWeek > 6) [](start = 11, length = 46)
should we support the value 7 for Sunday too? I mean 0 and 7 is Sunday.
I can see the scenario of someone getting the data from some source and just cast the value to DayOfWeek.
There was a problem hiding this comment.
I'm not sure. We won't be able to reuse the existing exception message (ArgumentOutOfRange_DayOfWeek):
The DayOfWeek enumeration must be in the range 0 through 6.
There was a problem hiding this comment.
I'm not sure. We won't be able to reuse the existing exception message (ArgumentOutOfRange_DayOfWeek):
Why? what I am trying to Say, if someone passed dayOfWeek = 7 to us, we'll treat it as Sunday. so we'll not throw. I don't think this affects the exception thrown for other cases. right?
There was a problem hiding this comment.
And we can document the behavior. also, we can have a new exception message as needed too.
There was a problem hiding this comment.
Alright. Let's also accept 7 😄
|
@khellang thanks again for getting the implementation ready. other than my comments, LGTM |
|
@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test |
* Added ISOWeek to System.Globalization * Revert DateTime changes. Use constants from GregorianCalendar instead. * Add more comments * Also allow 7 as value for day of week * Add note about ISO week-numbering year parameters * Add note about allowing 7 for day of week Signed-off-by: dotnet-bot <[email protected]>
* Added ISOWeek to System.Globalization * Revert DateTime changes. Use constants from GregorianCalendar instead. * Add more comments * Also allow 7 as value for day of week * Add note about ISO week-numbering year parameters * Add note about allowing 7 for day of week Signed-off-by: dotnet-bot <[email protected]>
* Added ISOWeek to System.Globalization * Revert DateTime changes. Use constants from GregorianCalendar instead. * Add more comments * Also allow 7 as value for day of week * Add note about ISO week-numbering year parameters * Add note about allowing 7 for day of week Signed-off-by: dotnet-bot-corefx-mirror <[email protected]>
* Added ISOWeek to System.Globalization * Revert DateTime changes. Use constants from GregorianCalendar instead. * Add more comments * Also allow 7 as value for day of week * Add note about ISO week-numbering year parameters * Add note about allowing 7 for day of week Signed-off-by: dotnet-bot-corefx-mirror <[email protected]>
|
Isn't |
|
Capitalization was noted in review : https://github.com/dotnet/corefx/issues/28933#issuecomment-392873426 |
|
Sorry @danmosemsft, I as on the phonw. |
* Added ISOWeek to System.Globalization * Revert DateTime changes. Use constants from GregorianCalendar instead. * Add more comments * Also allow 7 as value for day of week * Add note about ISO week-numbering year parameters * Add note about allowing 7 for day of week Commit migrated from dotnet/coreclr@79b8f94
5 participants
Part of https://github.com/dotnet/corefx/issues/28933
// @tarekgh