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

[Essentials] Add DateTimeOffset overload in Preferences #22815

Merged
merged 10 commits into from
Jun 25, 2024

Conversation

Cheesebaron
Copy link
Contributor

@Cheesebaron Cheesebaron commented Jun 4, 2024

Description of Change

Adds overload in Preferences to store and retrieve DateTimeOffset.

There is no ToBinary() method on DateTimeOffset so I am storing it as a ###string.

I have an issue with the roslyn analyzer that checks for PublicAPI, it keeps complaining no matter what I add to either file. ~~

Issues Fixed

Fixes #22786

@Cheesebaron Cheesebaron requested a review from a team as a code owner June 4, 2024 07:02
Copy link
Contributor

Hey there @Cheesebaron! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Jun 4, 2024
@jfversluis
Copy link
Member

I have an issue with the roslyn analyzer that checks for PublicAPI, it keeps complaining no matter what I add to either file.

Yeah its kind of annoying, but it probably keeps complaining because you have to add it to every txt file for every platform where this API is introduced.

@Cheesebaron
Copy link
Contributor Author

I have an issue with the roslyn analyzer that checks for PublicAPI, it keeps complaining no matter what I add to either file.

Yeah its kind of annoying, but it probably keeps complaining because you have to add it to every txt file for every platform where this API is introduced.

Do I add it to .Shipped or .Unshipped?

@mattleibow
Copy link
Member

Unshipped.

@Cheesebaron
Copy link
Contributor Author

Ah let me fix that

@Cheesebaron Cheesebaron force-pushed the feature/gh-22786-datetimeoffset branch from d33199f to 38f058c Compare June 4, 2024 10:38
@mattleibow
Copy link
Member

I see you are using ToString("O") - how does this work when the culture or timezone changes on the device? I don't see docs on the O format, and is it better to save using invariant culture and then a parse exact?

@dotnet dotnet deleted a comment from azure-pipelines bot Jun 4, 2024
@Cheesebaron
Copy link
Contributor Author

I see you are using ToString("O") - how does this work when the culture or timezone changes on the device? I don't see docs on the O format, and is it better to save using invariant culture and then a parse exact?

Looking at standard format strings here: https://learn.microsoft.com/en-us/dotnet/standard/base-types/standard-date-and-time-format-strings

O format saves in ISO 8601 which doesn't affect the format regardless of culture. It is also the only format that also contains the offset part.

mattleibow
mattleibow previously approved these changes Jun 4, 2024
@mattleibow
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@jsuarezruiz
Copy link
Contributor

/azp run

@jsuarezruiz jsuarezruiz added the area-essentials Essentials: Device, Display, Connectivity, Secure Storage, Sensors, App Info label Jun 4, 2024
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@Cheesebaron
Copy link
Contributor Author

Sorry for the many pushes, I can't build UWP on my machine 😢

@mattleibow
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@mattleibow
Copy link
Member

Sorry for the many pushes, I can't build UWP on my machine 😢

No prob, I also do CI driven dev because I am to lazy to boot a windows machine or launch a new devbox.

@mattleibow
Copy link
Member

mattleibow commented Jun 6, 2024

Windows is having a few failures:

System.InvalidCastException : Invalid cast from 'System.String' to 'System.DateTimeOffset'.

Stack trace

   at System.Convert.DefaultToType(IConvertible value, Type targetType, IFormatProvider provider)
   at Microsoft.Maui.Storage.UnpackagedPreferencesImplementation.Get[T](String key, T defaultValue, String sharedName) in /_/src/Essentials/src/Preferences/Preferences.uwp.cs:line 210
   at Microsoft.Maui.Storage.PreferencesImplementation.Get[T](String key, T defaultValue, String sharedName) in /_/src/Essentials/src/Preferences/Preferences.uwp.cs:line 37
   at Microsoft.Maui.Essentials.DeviceTests.Preferences_Tests.DateTimeOffsetPreservesOffset_NonStatic(String sharedName, TimeSpan offset) in /_/src/Essentials/test/DeviceTests/Tests/Preferences_Tests.cs:line 497
   at InvokeStub_Preferences_Tests.DateTimeOffsetPreservesOffset_NonStatic(Object, Span`1)
   at System.Reflection.MethodBaseInvoker.InvokeWithFewArgs(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)

Copy link
Member

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

Since this has a new API, it probably needs to target net9.0 instead.

@Cheesebaron Cheesebaron changed the base branch from main to net9.0 June 6, 2024 17:52
@Cheesebaron
Copy link
Contributor Author

Cheesebaron commented Jun 6, 2024

Windows is having a few failures:

System.InvalidCastException : Invalid cast from 'System.String' to 'System.DateTimeOffset'.

Stack trace

   at System.Convert.DefaultToType(IConvertible value, Type targetType, IFormatProvider provider)
   at Microsoft.Maui.Storage.UnpackagedPreferencesImplementation.Get[T](String key, T defaultValue, String sharedName) in /_/src/Essentials/src/Preferences/Preferences.uwp.cs:line 210
   at Microsoft.Maui.Storage.PreferencesImplementation.Get[T](String key, T defaultValue, String sharedName) in /_/src/Essentials/src/Preferences/Preferences.uwp.cs:line 37
   at Microsoft.Maui.Essentials.DeviceTests.Preferences_Tests.DateTimeOffsetPreservesOffset_NonStatic(String sharedName, TimeSpan offset) in /_/src/Essentials/test/DeviceTests/Tests/Preferences_Tests.cs:line 497
   at InvokeStub_Preferences_Tests.DateTimeOffsetPreservesOffset_NonStatic(Object, Span`1)
   at System.Reflection.MethodBaseInvoker.InvokeWithFewArgs(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)

Gotta fire up my Windows machine to see what is up here. Thanks!

I need to figure out what is up with this UnpackagedPreferencesImplementation and why it behaves as it does.

@rmarinho rmarinho requested a review from mattleibow June 6, 2024 22:38
@Cheesebaron
Copy link
Contributor Author

So seems like DateTime was skipped in the device tests for unpackaged UWP. I've gone ahead and fixed that so that test isn't skipped and made sure DateTimeOffset tests also work. If this is not what you want I can also just skip the DateTimeOffset tests on upackaged UWP. Let me know which direction you want to go.

@jsuarezruiz
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@Cheesebaron
Copy link
Contributor Author

Looks like the failing pipelines are unrelated to the changes in this PR

@mattleibow
Copy link
Member

Looking good, now to see what is going on with ci and UI tests

@Cheesebaron
Copy link
Contributor Author

Not to be pushy, but any chance this gets in before .NET 9 comes out of preview?

@mattleibow mattleibow merged commit 5b3fd9a into dotnet:net9.0 Jun 25, 2024
42 of 50 checks passed
@Cheesebaron Cheesebaron deleted the feature/gh-22786-datetimeoffset branch June 25, 2024 15:28
@github-actions github-actions bot locked and limited conversation to collaborators Jul 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-essentials Essentials: Device, Display, Connectivity, Secure Storage, Sensors, App Info community ✨ Community Contribution fixed-in-9.0.0-preview.6.24327.7
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add DateTimeOffset overload to Preferences
5 participants