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

DYN-7471 Add compatibility matrix deserialization #113

Merged
merged 1 commit into from
Sep 25, 2024

Conversation

zeusongit
Copy link
Contributor

@zeusongit zeusongit commented Sep 18, 2024

Purpose

Adding compatibility matrix to PMClient with deserialization so Dynamo gets a collection of different compatibility info instead of a string.

The PR introduces changes to the compatibility schema:
The changes are not huge, it is proposed to change the json structure from a dictionary to an array of objects, reason behind this is since we need to deserialize this json property into a dictionary, we will also need to convert it into an observable collection when consuming it in Dynamo, an Observable Dictionary is more complex and will probably require creating this Observable Dictionary class from scratch, I feel using an already existing Observable Collection makes more sense.
The schema wiki has been updated with the new proposal, also added a test for deserialization.

Declarations

Check these if you believe they are true

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.

Reviewers

(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)

(FILL ME IN, optional) Any additional notes to reviewers or testers.

FYIs

(FILL ME IN, Optional) Names of anyone else you wish to be notified of

Copy link
Contributor

@QilongTang QilongTang left a comment

Choose a reason for hiding this comment

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

LGTM, have you communicated with @deepakanand on this?

@zeusongit
Copy link
Contributor Author

LGTM, have you communicated with @deepakanand on this?

I think they are not using it yet, or still using mocked data.

@QilongTang
Copy link
Contributor

LGTM, have you communicated with @deepakanand on this?

I think they are not using it yet, or still using mocked data.

Thanks

@zeusongit zeusongit merged commit 3109720 into DynamoDS:master Sep 25, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants