Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Force short date pattern to use yyyy on Linux #18316

Merged
merged 2 commits into from
Jun 7, 2018
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -134,16 +134,103 @@ private static bool EnumDatePatterns(string localeName, CalendarId calendarId, C
{
List<string> datePatternsList = callbackContext.Results;

datePatterns = new string[datePatternsList.Count];
for (int i = 0; i < datePatternsList.Count; i++)
{
datePatterns[i] = NormalizeDatePattern(datePatternsList[i]);
datePatternsList[i] = NormalizeDatePattern(datePatternsList[i]);
}

if (dataType == CalendarDataType.ShortDates)
FixDefaultShortDatePattern(datePatternsList);

datePatterns = datePatternsList.ToArray();
}

return result;
}

// FixDefaultShortDatePattern will convert the default short date pattern from using 'yy' to using 'yyyy'
// And will ensure the original pattern still exist in the list
private static void FixDefaultShortDatePattern(List<string> shortDatePatterns)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be missing context but are we assuming there will be only one 'yy'?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. if we have more than one yy that will be ok because we'll convert at least one of them which will show the 4 digit year

{
if (shortDatePatterns.Count == 0)
return;

string s = shortDatePatterns[0];

if (s.Length > 100)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A comment about why 100 is special would help.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to pick some number to ensure we'll not cause a stack overflow. 100 is more than enough for the date pattern but I'll add a comment

return;

Span<char> modifiedPattern = stackalloc char[s.Length + 2];
int index = 0;

while (index < s.Length)
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are ticks getting special treatment? A comment with some example inputs and outputs might help.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what you mean by ticks here. I thought the comment before the method should be clear. I'll try to clarify it. basically, if try to format any date with the short pattern on Linux the year will be displayed as 2 digits. "6/5/18" this formatting has nothing to do with the date ticks. the fix is to display the year as 4 digits

if (s[index] == '\'')
{
do
{
modifiedPattern[index] = s[index];
index++;
} while (index < s.Length && s[index] != '\'');

if (index >= s.Length)
return;
}
else if (s[index] == 'y')
{
modifiedPattern[index] = 'y';
break;
}

modifiedPattern[index] = s[index];
index++;
}

if (index >= s.Length - 1 || s[index + 1] != 'y')
Copy link
Member

@krwq krwq Jun 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

index + 1 >= s.Length would make it easier to see there is no edge case here, if you increment index before it might also simplify the rest a little bit

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for code readability, what I have is better is you read it we are checking where the index now and if we crossed the boundaries. I prefer to keep the way I wrote but let me know if you still feel strongly about it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for this one not strongly

{
// not a 'yy' pattern
return;
}

if (index <= s.Length - 3 && s[index + 2] == 'y')
Copy link
Member

@krwq krwq Jun 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

index + 2 < s.Length? Also consider incrementing before

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto as last comment

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for this one it would be easier to read if you use 2

{
// we have 'yyy' then nothing to do
return;
}

// we are sure now we have 'yy' pattern

Debug.Assert(index + 3 < modifiedPattern.Length);

modifiedPattern[index + 1] = 'y'; // second y
modifiedPattern[index + 2] = 'y'; // third y
modifiedPattern[index + 3] = 'y'; // fourth y

index += 2;

// Now, copy the rest of the pattern to the destination buffer
while (index < s.Length)
{
modifiedPattern[index + 2] = s[index];
index++;
}

shortDatePatterns[0] = modifiedPattern.ToString();

for (int i=1; i < shortDatePatterns.Count; i++)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: spaces around =

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix that

{
if (shortDatePatterns[i] == shortDatePatterns[0])
{
// Found match in the list to the new constructed pattern, then replace it with the original modified pattern
shortDatePatterns[i] = s;
return;
}
}

// if we come here means the newly constructed pattern not found on the list, then add the original pattern
shortDatePatterns.Add(s);
}

/// <summary>
/// The ICU date format characters are not exactly the same as the .NET date format characters.
/// NormalizeDatePattern will take in an ICU date pattern and return the equivalent .NET date pattern.
Expand Down