Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[mono][HybridGlobalization] Fix ShortDatePattern year format to be "yyyy" #99908

Merged
merged 11 commits into from Apr 9, 2024

Conversation

matouskozak
Copy link
Member

@matouskozak matouskozak commented Mar 18, 2024

The globalization DateTimeFormat.ShortDatePattern API is returning short year format instead of "yyyy" format when Hybrid Globalization is enabled on iOS (previously discussed for Linux in #26088). This PR overwrites the short year format ("y"/"yy") with "yyyy".

Fixes #99515. Needs backport to .NET 8.


Contributed: @mkhamoyan

@matouskozak matouskozak added this to the 9.0.0 milestone Mar 18, 2024
@matouskozak matouskozak self-assigned this Mar 18, 2024
Copy link
Contributor

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

Copy link
Contributor

Tagging subscribers to 'os-ios': @steveisok, @akoeplinger, @kotlarmilos
See info in area-owners.md if you want to be subscribed.

@tipa
Copy link

tipa commented Mar 19, 2024

Thanks for fixing this so quickly. Can it be ruled out that the iOS native APIs return "yyyy" as the shortFormatString? Because if the native API returns "yyyy", I think the added code would replace it with "yyyyyyyy"

@matouskozak
Copy link
Member Author

Thanks for fixing this so quickly. Can it be ruled out that the iOS native APIs return "yyyy" as the shortFormatString? Because if the native API returns "yyyy", I think the added code would replace it with "yyyyyyyy"

Yes, you are right. It seems that the iOS native APIs do return only "y"/"yy" for the tested scenarios but I will push a change so that we can be safe.

@akoeplinger
Copy link
Member

I haven't looked too closely but why is FixDefaultShortDatePattern in managed not kicking in? I'd prefer reusing that instead of dealing with it in native code.

@mkhamoyan
Copy link
Member

I haven't looked too closely but why is FixDefaultShortDatePattern in managed not kicking in? I'd prefer reusing that instead of dealing with it in native code.

Currently FixDefaultShortDatePattern is not called for hybrid mode

#elif TARGET_MACCATALYST || TARGET_IOS || TARGET_TVOS
return GlobalizationMode.Hybrid ?
LoadCalendarDataFromNative(localeName, calendarId) :
IcuLoadCalendarDataFromSystem(localeName, calendarId);

We can reuse FixDefaultShortDatePattern, but we will need to modify it as apple native api now returns "y" or "yy".

@matouskozak
Copy link
Member Author

I haven't looked too closely but why is FixDefaultShortDatePattern in managed not kicking in? I'd prefer reusing that instead of dealing with it in native code.

Currently FixDefaultShortDatePattern is not called for hybrid mode

#elif TARGET_MACCATALYST || TARGET_IOS || TARGET_TVOS
return GlobalizationMode.Hybrid ?
LoadCalendarDataFromNative(localeName, calendarId) :
IcuLoadCalendarDataFromSystem(localeName, calendarId);

We can reuse FixDefaultShortDatePattern, but we will need to modify it as apple native api now returns "y" or "yy".

Yes, that is right. I was thinking about tinkering the FixDefaultShortDatePattern so that it works for "y" case but I wasn't sure if it is the best approach as it is used outside Hybrid as well and we will need backport to .NET 8.

Let me know if you would prefer this approach and I can give it a go.

@matouskozak matouskozak added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 28, 2024
@matouskozak
Copy link
Member Author

@tarekgh we probably hit the issue that different Apple ICU version give different outputs you mentioned here #100240 (comment). I plan to find a set of "stable" locals that I would use for testing. Do you think it would make sense to find a small set that could be used for ICU (non-hybrid) as well so that we would have coverage across Globalization backends or should I only focus on Apple hybrid in this PR? (Note: I planned to backport this PR to .NET 8)

@tarekgh
Copy link
Member

tarekgh commented Mar 28, 2024

@matouskozak if en-US and fr-FR are not stable, I doubt any other cultures will be stable :-) these are the two culture that are sensitive to changes. I would suggest special case the hybrid.

@matouskozak
Copy link
Member Author

/azp run runtime-extra-platforms

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@matouskozak
Copy link
Member Author

@matouskozak if en-US and fr-FR are not stable, I doubt any other cultures will be stable :-) these are the two culture that are sensitive to changes. I would suggest special case the hybrid.

Thanks. I tried invariant, en-US and fr-FR and they seem to be stable for now. For now, I only enabled it for Apple hybrid, we could later enable it for non-hybrid/ICU globalization as well (tested locally and it should work).

@matouskozak matouskozak marked this pull request as ready for review March 31, 2024 13:47
@matouskozak matouskozak merged commit 404b286 into dotnet:main Apr 9, 2024
147 of 150 checks passed
@matouskozak
Copy link
Member Author

/backport to release/8.0-staging

Copy link
Contributor

github-actions bot commented Apr 9, 2024

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/8611432872

Copy link
Contributor

github-actions bot commented Apr 9, 2024

@matouskozak an error occurred while backporting to release/8.0-staging, please check the run log for details!

Error: @matouskozak is not a repo collaborator, backporting is not allowed. If you're a collaborator please make sure your dotnet team membership visibility is set to Public on https://github.com/orgs/dotnet/people?query=matouskozak

Copy link
Contributor

github-actions bot commented Apr 9, 2024

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/8611432872

Copy link
Contributor

github-actions bot commented Apr 9, 2024

@matouskozak backporting to release/8.0-staging failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: use yyyy year format for short date pattern
Applying: add test for yyyy short date pattern
Using index info to reconstruct a base tree...
A	src/libraries/System.Runtime/tests/System.Globalization.Tests/DateTimeFormatInfo/DateTimeFormatInfoShortDatePattern.cs
Falling back to patching base and 3-way merge...
CONFLICT (modify/delete): src/libraries/System.Runtime/tests/System.Globalization.Tests/DateTimeFormatInfo/DateTimeFormatInfoShortDatePattern.cs deleted in HEAD and modified in add test for yyyy short date pattern. Version add test for yyyy short date pattern of src/libraries/System.Runtime/tests/System.Globalization.Tests/DateTimeFormatInfo/DateTimeFormatInfoShortDatePattern.cs left in tree.
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 add test for yyyy short date pattern
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

Copy link
Contributor

github-actions bot commented Apr 9, 2024

@matouskozak an error occurred while backporting to release/8.0-staging, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

matouskozak added a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024
…yyy" (dotnet#99908)

* use yyyy year format for short date pattern

* add test for yyyy short date pattern

---------

Co-authored-by: Ilona Tomkowicz <32700855+ilonatommy@users.noreply.github.com>
@github-actions github-actions bot locked and limited conversation to collaborators May 9, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Globalization NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) os-ios Apple iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[iOS/HybridGlobalization] Force short date pattern to use yyyy
6 participants