Fix reading Time zone rules using Julian days#17635
Conversation
|
@eerhardt could you please have a look? thank you. |
|
@dotnet-bot test this |
1 similar comment
|
@dotnet-bot test this |
|
@dotnet/dnceng please check this. it is failed many times for unrelated reasons. |
|
@tarekgh What failed here? |
|
I assume a test is coming in corefx? |
| month = day = 0; | ||
|
|
||
| int index = 0; | ||
| bool isLeapYear = true; |
There was a problem hiding this comment.
Instead of isLeapYear, I'd call this shouldIncludeLeapDay.
There was a problem hiding this comment.
We may change this or remove it according to what we'll find about the n format
| int[] days = isLeapYear ? GregorianCalendarHelper.DaysToMonth366 : GregorianCalendarHelper.DaysToMonth365; | ||
|
|
||
| throw new InvalidTimeZoneException(SR.InvalidTimeZone_JulianDayNotSupported); | ||
| if (julianDay == 0 || julianDay > days[days.Length - 1]) |
There was a problem hiding this comment.
According to http://man7.org/linux/man-pages/man3/tzset.3.html, julianDay == 0 is valid for the case where the J is not specified.
n This specifies the zero-based Julian day with n between 0 and 365. February 29 is counted in leap years.
There was a problem hiding this comment.
As we have chatted, I'll try to research how ```n```` format should be used
| { | ||
| julianDay = julianDay * 10 + (int) (date[index] - '0'); | ||
| index++; | ||
| } while (index < date.Length && ((uint)(date[index] - '0') <= '9'-'0')); |
There was a problem hiding this comment.
Is it not possible to use int.TryParse here?
There was a problem hiding this comment.
It is possible but my implementation should be more performant.
|
@mmitche sorry, it was failing in all legs before I closed the PR, but after re-opening it, looks everything is good. |
| { | ||
| // Jn | ||
| // This specifies the Julian day, with n between 1 and 365.February 29 is never counted, even in leap years. | ||
| Debug.Assert(!String.IsNullOrEmpty(date)); |
There was a problem hiding this comment.
Another good assert would be that date[0] == 'J', since we are skipping the first char below with int index = 1;
| // For example if n is specified as 60, this means in leap year the rule will start at Mar 1, | ||
| // while in non leap year the rule will start at Mar 2. | ||
| // | ||
| // If we need to support n format, we'll have to have a floating adjustment rule support this case. |
There was a problem hiding this comment.
Should we open an issue for this? or should we wait until we get more feedback to support this format?
There was a problem hiding this comment.
I'll open a future issue anyway so we may pro-actively fixing it if we get a chance.
|
@mmitche does the OSX build seems right? I am seeing the following and the progress bar is really at the begining. |
|
test OSX10.12 x64 Checked Innerloop Build and Test please |
* Fix reading Time zone rules using Julian days * Order the condition correctly * Start searching from month 1 and not 0 * Exclude n Julian format * fix typo * Adding the suggested assert Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>
* Fix reading Time zone rules using Julian days * Order the condition correctly * Start searching from month 1 and not 0 * Exclude n Julian format * fix typo * Adding the suggested assert Signed-off-by: dotnet-bot-corefx-mirror <dotnet-bot@microsoft.com>
|
@tarekgh does this need to go into 2.1? |
|
@danmosemsft |
* Fix reading Time zone rules using Julian days * Order the condition correctly * Start searching from month 1 and not 0 * Exclude n Julian format * fix typo * Adding the suggested assert Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
* Fix reading Time zone rules using Julian days * Order the condition correctly * Start searching from month 1 and not 0 * Exclude n Julian format * fix typo * Adding the suggested assert Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
5 participants
Fixes #17393