Update Value of TwoDigitYearMax to 2049 by cdbullard · Pull Request #76848 · dotnet/runtime

cdbullard · 2022-10-11T00:28:51Z

Update the century assumption cutoff year for determining whether a year is referring to a 19XX date or a 20XX date from 2029 to 2049.

ghost · 2022-10-11T00:29:06Z

Tagging subscribers to this area: @dotnet/area-system-globalization
See info in area-owners.md if you want to be subscribed.

Issue Details

Fix #75148

Update the century assumption cutoff year for determining whether a year is referring to a 19XX date or a 20XX date from 2029 to 2049.

Author:	cdbullard
Assignees:	-
Labels:	`area-System.Globalization`
Milestone:	-

src/libraries/System.Private.CoreLib/src/System/Globalization/CalendarData.cs

src/libraries/System.Private.CoreLib/src/System/Globalization/EastAsianLunisolarCalendar.cs

danmoseley · 2022-10-11T01:28:01Z

Tests are all under System.Globalization.Calendars. Search for "twodigityear" eg

runtime/src/libraries/System.Globalization.Calendars/tests/GregorianCalendar/GregorianCalendarTwoDigitYearMax.cs

Line 25 in dd6696a

    
           Assert.True(calendar.TwoDigitYearMax == 2029 || calendar.TwoDigitYearMax == 2049, $"Unexpected calendar.TwoDigitYearMax {calendar.TwoDigitYearMax}");

src/libraries/System.Globalization.Calendars/System.Globalization.Calendars.sln

cdbullard · 2022-10-11T03:50:48Z

Interestingly when I try to change this to 2049 and 2069, the unit test fails:

runtime/src/libraries/System.Globalization.Calendars/tests/GregorianCalendar/GregorianCalendarTwoDigitYearMax.cs

Line 25 in dd6696a

    
           Assert.True(calendar.TwoDigitYearMax == 2029 || calendar.TwoDigitYearMax == 2049, $"Unexpected calendar.TwoDigitYearMax {calendar.TwoDigitYearMax}");

@danmoseley Just wanted to leave a note, this section is giving me trouble with updating the unit tests (and others seem to cause similar issues, even with the updated code). Do you have any advice?

src/libraries/System.Private.CoreLib/src/System/Globalization/CalendarData.cs

danmoseley · 2022-10-11T18:38:51Z

build failures are fixed by #76860

tarekgh · 2022-10-11T18:54:19Z

I am not seeing any change to read the two digits max settings when running on Windows (when using ICU).

I mean the code at

runtime/src/libraries/System.Private.CoreLib/src/System/Globalization/Calendar.cs

Line 714 in 20d4e30

    
           int twoDigitYearMax = GlobalizationMode.UseNls ? CalendarData.NlsGetTwoDigitYearMax(CalID) : CalendarData.IcuGetTwoDigitYearMax();

Need to change. Instead of branching for NLS and ICU cases, we need to branch it for Windows and non-Windows` case (with considering the special case of invariant mode).
So, on Windows we'll call

runtime/src/libraries/System.Private.CoreLib/src/System/Globalization/CalendarData.Nls.cs

Line 14 in 20d4e30

internal static int NlsGetTwoDigitYearMax(CalendarId calendarId)

and on none-Windows we'll do

runtime/src/libraries/System.Private.CoreLib/src/System/Globalization/CalendarData.Icu.cs

Line 86 in 20d4e30

Debug.Assert(!GlobalizationMode.UseNls);

Usually, we can have these two different implementations in CalendarData.Unix.cs and CalendarData.Windows.cs files.

danmoseley · 2022-10-11T20:26:46Z

some extra context here @cdbullard, although you probably know

on non Windows, we use the ICU library for globalization settings. we don't read two digit year value from ICU, apparently it doesn't exist in ICU, so we use the hard coded default you changed.
on Windows we also use ICU, so again we have to use the hard coded default. however, you can switch your app to NLS mode if you want, and in this mode is needs to read the Windows setting you can change in the Window UI. This is the behavior today and should continue.

This has info on how to switch to NLS mode. Before your change, you should notice that in NLS mode, when you change the Windows two digit year value in the Windows UI, your C# code will see the change. That's the behavior that needs to stay.
https://learn.microsoft.com/en-US/dotnet/core/runtime-config/globalization

tarekgh · 2022-10-11T23:17:58Z

src/libraries/System.Private.CoreLib/src/System/Globalization/Calendar.cs

        internal static int GetSystemTwoDigitYearSetting(CalendarId CalID, int defaultYearValue)
        {
-            int twoDigitYearMax = GlobalizationMode.UseNls ? CalendarData.NlsGetTwoDigitYearMax(CalID) : CalendarData.IcuGetTwoDigitYearMax();
+            int twoDigitYearMax = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? CalendarData.NlsGetTwoDigitYearMax(CalID) : CalendarData.IcuGetTwoDigitYearMax();


Thanks @cdbullard for adding that. We usually avoid doing the OS checks inside the code. Instead we separate the code during compile time.

For this change, please move the method NlsGetTwoDigitYearMax to the file CalendarData.Windows.cs and rename the method to something like GetTwoDigitYearMax. Then move the method IcuGetTwoDigitYearMax to the file CalendarData.Unix.cs and rename the method to the same name GetTwoDigitYearMax (taking same parameters as the one moved to Windows file). last, make GetSystemTwoDigitYearSetting just CalendarData.GetTwoDigitYearMax.

Thanks for clarifying that for me. If I make the method GetSystemTwoDigitYearSetting like this:

internal static int GetSystemTwoDigitYearSetting(CalendarId CalID, int defaultYearValue) { return CalendarData.GetTwoDigitYearMax(CalID); }

should I also remove the defaultYearValue from the signature and from the places this method is being called?

No, please keep it as:

internal static int GetSystemTwoDigitYearSetting(CalendarId CalID, int defaultYearValue) { int twoDigitYearMax = CalendarData.GetTwoDigitYearMax(CalID); return twoDigitYearMax >= 0 ? twoDigitYearMax : defaultYearValue; }

Sorry I was not clear in that part :-)

Thanks! I've just pushed a commit for this, let me know if any of it should be changed

…dows/Unix)

tarekgh · 2022-10-11T23:59:24Z

src/libraries/System.Private.CoreLib/src/System/Globalization/CalendarData.Unix.cs

+
+            // There is no user override for this value on Linux or in ICU.
+            // So just return -1 to use the hard-coded defaults.
+            return -1;


return -1;

Could you please change this to:

return GlobalizationMode.Invariant ? Invariant.iTwoDigitYearMax : -1;

Sure thing, this has been corrected

please check there's a test that reads this in invariant mode (ie that would fail without the change above)

tarekgh · 2022-10-12T00:01:58Z

@cdbullard I added one more little comment, LGTM otherwise. Let's see if all tests pass and then we merge it.

ghost · 2022-10-12T01:40:40Z

Added needs-breaking-change-doc-created label because this PR has the breaking-change label.

When you commit this breaking change:

Create and link to this PR and the issue a matching issue in the dotnet/docs repo using the breaking change documentation template, then remove this needs-breaking-change-doc-created label.
Ask a committer to mail the .NET Breaking Change Notification DL.

Breaking change doc

Tagging @dotnet/compat for awareness of the breaking change.

tarekgh · 2022-10-12T01:49:33Z

@cdbullard

src\libraries\System.Private.CoreLib\src\System\Globalization\CalendarData.Nls.cs(26,28): error CA1823: (NETCORE_ENGINEERING_TELEMETRY=Build) Unused field 'CAL_ITWODIGITYEARMAX'

please remove CAL_ITWODIGITYEARMAX from that file and ensure it is defined in the CalendarData.Windows.cs file.

src/libraries/System.Private.CoreLib/src/System/Globalization/CalendarData.cs

…CalendarData.cs Co-authored-by: Dan Moseley <danmose@microsoft.com>

…nto twoyearvalue

danmoseley · 2022-10-13T13:54:45Z

did you merge away some of your tests? I only see one

tarekgh · 2022-10-13T15:55:05Z

src/libraries/System.Private.CoreLib/src/System/Globalization/EastAsianLunisolarCalendar.cs

                if (_twoDigitYearMax == -1)
                {
-                    _twoDigitYearMax = GetSystemTwoDigitYearSetting(BaseCalendarID, GetYear(new DateTime(DefaultGregorianTwoDigitYearMax, 1, 1)));
+                    _twoDigitYearMax = GetSystemTwoDigitYearSetting(ID, DefaultGregorianTwoDigitYearMax);


Why did you change this? GetYear gets the year from the used calendar and not necessary to match the Gregorian year.

This change was discussed under this comment, but let me know if it was handled incorrectly in my commit

I replied to that comment. Please try to revert this change.

that was my bad.

tarekgh · 2022-10-22T22:23:26Z

Thanks @cdbullard for helping with this change!

cdbullard · 2022-10-23T00:58:50Z

My pleasure @tarekgh! Will keep an eye out for more :-)

cdbullard added 3 commits September 8, 2022 21:41

Initial commit

d132f77

Merge branch 'dotnet:main' into twoyearvalue

578ee02

Merge branch 'dotnet:main' into twoyearvalue

43ebf1a

ghost added area-System.Globalization community-contribution Indicates that the PR has been added by a community member labels Oct 11, 2022

cdbullard mentioned this pull request Oct 11, 2022

Consider increase to TwoDigitYearMax #75148

Closed

danmoseley reviewed Oct 11, 2022

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Globalization/CalendarData.cs Outdated Show resolved Hide resolved

danmoseley reviewed Oct 11, 2022

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Globalization/EastAsianLunisolarCalendar.cs Show resolved Hide resolved

Updating unit tests

4873c8b

danmoseley reviewed Oct 11, 2022

View reviewed changes

src/libraries/System.Globalization.Calendars/System.Globalization.Calendars.sln Outdated Show resolved Hide resolved

cdbullard added 4 commits October 10, 2022 23:36

Removing .sln changes

a43ef8b

Update ThaiBuddhistCalendarToFourDigitYear.cs

e5a0630

Update System.Globalization.sln

46950ac

Update System.Globalization.Calendars.sln

635d695

build-analysis bot mentioned this pull request Oct 11, 2022

Assertion failed: (*card_word)==0 in DynamicGenerics tests #76801

Closed

danmoseley reviewed Oct 11, 2022

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Globalization/CalendarData.cs Outdated Show resolved Hide resolved

cdbullard added 2 commits October 11, 2022 19:01

Remove Y2K and Marshaller comments

ab97ec0

Adding in OS check for determining whether to use ICU or NLS

527dca2

tarekgh reviewed Oct 11, 2022

View reviewed changes

Refactoring code for determining which two digit year max to use (Win…

ee3b20b

…dows/Unix)

tarekgh reviewed Oct 11, 2022

View reviewed changes

Update CalendarData.Unix.cs

91cd057

danmoseley added the breaking-change Issue or PR that represents a breaking API or functional change over a previous release. label Oct 12, 2022

danmoseley added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Oct 12, 2022

Move CAL_ITWODIGITYEARMAX;

a243ade

This was referenced Oct 12, 2022

Tracking Nuget 429s dotnet/arcade#10885

Closed

Nuget restore error caused by 503s dotnet/arcade#11239

Closed

danmoseley reviewed Oct 12, 2022

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Globalization/CalendarData.cs Outdated Show resolved Hide resolved

cdbullard and others added 3 commits October 12, 2022 10:20

Update src/libraries/System.Private.CoreLib/src/System/Globalization/…

7cfd57a

…CalendarData.cs Co-authored-by: Dan Moseley <danmose@microsoft.com>

Updating EastAsianLunisolarCalendar callout

ffc8020

Merge branch 'twoyearvalue' of https://github.com/cdbullard/runtime i…

a4b25f9

…nto twoyearvalue

tarekgh reviewed Oct 13, 2022

View reviewed changes

Update EastAsianLunisolarCalendar.cs

c0f7752

tarekgh mentioned this pull request Oct 22, 2022

[Breaking change]: TwoDigitYearMax default value changed from 2029 to 2049 in Gregorian based calendars dotnet/docs#31978

Closed

2 tasks

tarekgh approved these changes Oct 22, 2022

View reviewed changes

tarekgh merged commit ebf4ea7 into dotnet:main Oct 22, 2022

tarekgh removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Oct 22, 2022

tarekgh added this to the 8.0.0 milestone Oct 22, 2022

cdbullard deleted the twoyearvalue branch October 23, 2022 01:32

jakobbotsch mentioned this pull request Oct 25, 2022

Assertion failure "GlobalizationMode.UseNls" in CalendarData.GetTwoDigitYearMax #77451

Closed

ghost locked as resolved and limited conversation to collaborators Nov 22, 2022

Conversation

cdbullard commented Oct 11, 2022

Uh oh!

ghost commented Oct 11, 2022

Uh oh!

Uh oh!

Uh oh!

danmoseley commented Oct 11, 2022

Uh oh!

Uh oh!

cdbullard commented Oct 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

danmoseley commented Oct 11, 2022

Uh oh!

tarekgh commented Oct 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danmoseley commented Oct 11, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tarekgh commented Oct 12, 2022

Uh oh!

ghost commented Oct 12, 2022 • edited by tarekgh Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tarekgh commented Oct 12, 2022

Uh oh!

Uh oh!

danmoseley commented Oct 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tarekgh commented Oct 22, 2022

Uh oh!

cdbullard commented Oct 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cdbullard commented Oct 11, 2022 •

edited

Loading

tarekgh commented Oct 11, 2022 •

edited

Loading

ghost commented Oct 12, 2022 •

edited by tarekgh

Loading