-
Notifications
You must be signed in to change notification settings - Fork 1.4k
SystemSerializer update for BaseObjectStorageHelper #3702
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
Conversation
…mInformation Tested copying data from 6.1 Sample App and was able to read/resume file writes with this new serializer. TODO: Update tests.
Thanks michael-hawker for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌 |
Hmm, the UITests failed... I'll try a full build locally to check. |
Just built locally fine: @azchohfi I think I've noticed this a couple of times where the UI Tests just fail still in the CI... anything we can do to figure out what might have caused it? |
This is the test that failed: Data of this type is not supported.
|
@azchohfi this test is passing locally it's supposed to throw an exception: It just looks like it's throwing a slightly different exception in the CI? ( I tried updating to use the more general
|
…OS? code paths throwing different Exception Types...
Hello @michael-hawker! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
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.
We forgot a small thing.
Fixes #3692
This introduces a new light-weight serializer which we can use within the toolkit for the
SystemInformation
class still. And then developers with more complex type needs can specify their own serializer for theBaseObjectStorageHelper
classes. See the related issue for more details.Follow-on to #3637 and #3414
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Using
DataContractJsonSerializer
, though we had usedNewtonsoft.Json
in 6.1.What is the new behavior?
Use new
SystemSerializer
which passes values to/from the WindowsApplicationDataContainer
APIs directly. Updates our internal usages of this to the new system. Requires developer using theBaseObjectStorageHelper
to provide a serializer implementation.This of course does provide a 'default'/system information within the toolkit now compared to our original thought of providing none at all before the
SystemInformation
problem was identified.PR Checklist
Please check if your PR fulfills the following requirements:
Other information
I tested copying the system.dat file from the Settings folder of our 6.1 sample app to the development version with the changes, no issues with
SystemInformation
reading the old data.Updated/added unit tests to test the old json layer vs. reading from the new SystemSerializer as well as just testing a new
System.Text.Json
complex scenario. Added tests for exceptions.TODO:
SystemSerializer
to handle a few more 'basic' (but non-primitive) types? E.g. @Sergio0694 has a similar implementation for a different related system