-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Force short date pattern to use yyyy on Linux #18316
Force short date pattern to use yyyy on Linux #18316
Conversation
The default pattern we get is using yy which causes the years to be displayed as 2 digits. This is not acceptable for many users. The fix here is to force 4-digit year as a default and still keep the original pattern which has 2-digits year in the optional list
@krwq @AlexGhiondea could you please help to review this one? Thanks. |
int index = 0; | ||
|
||
while (index < s.Length) | ||
{ |
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.
Why are ticks getting special treatment? A comment with some example inputs and outputs might help.
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.
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
|
||
string s = shortDatePatterns[0]; | ||
|
||
if (s.Length > 100) |
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.
A comment about why 100 is special would help.
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.
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
index++; | ||
} | ||
|
||
if (index >= s.Length - 1 || s[index + 1] != 'y') |
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.
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
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.
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.
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.
for this one not strongly
return; | ||
} | ||
|
||
if (index <= s.Length - 3 && s[index + 2] == 'y') |
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.
index + 2 < s.Length
? Also consider incrementing before
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.
ditto as last comment
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.
for this one it would be easier to read if you use 2
} | ||
|
||
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) |
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.
I might be missing context but are we assuming there will be only one 'yy'?
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.
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
|
||
shortDatePatterns[0] = modifiedPattern.ToString(); | ||
|
||
for (int i=1; i < shortDatePatterns.Count; i++) |
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: spaces around =
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.
I'll fix that
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.
added couple of comments
@dotnet-bot test Ubuntu x64 Checked corefx_baseline |
@dotnet/dnceng could you help to find out what is the problem with Ubuntu x64 Checked Build and Test (Jit - CoreFx)? The console log doesn't seem to be much useful |
Investigating as part of https://github.com/dotnet/core-eng/issues/3639 |
* Force short date pattern to use yyyy on Linux The default pattern we get is using yy which causes the years to be displayed as 2 digits. This is not acceptable for many users. The fix here is to force 4-digit year as a default and still keep the original pattern which has 2-digits year in the optional list * Address the review feedback Signed-off-by: dotnet-bot <[email protected]>
* Force short date pattern to use yyyy on Linux The default pattern we get is using yy which causes the years to be displayed as 2 digits. This is not acceptable for many users. The fix here is to force 4-digit year as a default and still keep the original pattern which has 2-digits year in the optional list * Address the review feedback Signed-off-by: dotnet-bot <[email protected]>
* Force short date pattern to use yyyy on Linux The default pattern we get is using yy which causes the years to be displayed as 2 digits. This is not acceptable for many users. The fix here is to force 4-digit year as a default and still keep the original pattern which has 2-digits year in the optional list * Address the review feedback Signed-off-by: dotnet-bot-corefx-mirror <[email protected]>
* Force short date pattern to use yyyy on Linux The default pattern we get is using yy which causes the years to be displayed as 2 digits. This is not acceptable for many users. The fix here is to force 4-digit year as a default and still keep the original pattern which has 2-digits year in the optional list * Address the review feedback Signed-off-by: dotnet-bot-corefx-mirror <[email protected]>
@tarekgh - I tried to use dotnet core build pack 2.1.3, it did not resolve my issue (still getting |
This is fixed in 3.0. we didn't change 2.1 behavior for app compatibility reason. |
* Force short date pattern to use yyyy on Linux The default pattern we get is using yy which causes the years to be displayed as 2 digits. This is not acceptable for many users. The fix here is to force 4-digit year as a default and still keep the original pattern which has 2-digits year in the optional list * Address the review feedback Commit migrated from dotnet/coreclr@a513283
The default pattern we get is using yy which causes the years to be displayed as 2 digits. This is not acceptable for many users. The fix here is to force 4-digit year as a default and still keep the original pattern which has 2-digit year in the optional list