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.

Sign up for GitHub

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

Jump to bottom

Refactoring Json serialization #209

Merged
merged 21 commits into from
May 23, 2023
Merged

Refactoring Json serialization #209

merged 21 commits into from
May 23, 2023

Conversation

Eneuman
Copy link
Contributor

@Eneuman Eneuman commented Mar 19, 2023 •
edited
Loading

This PR aims to make it easier to understand what serialization function should be used and where.

Serialization used for logging
If this serialization fails it should never crash the program.
Two methods handel this:

string SerializeForLoggingPurpose(object input)
string SerializeForLoggingPurposeIndented(object input)

Serialization used for running the program
If this serialization fails it should be logged by the function calling the serialization.
Four methods handel this:

T DeserializeObject<T>(string v)
T DeserializeObjectCaseInsensitive<T>(string v)
string SerializeObject(object obj)
string SerializeObjectIndented(object obj)

Other fixes

  • Lowered the memory allocations by making sure JsonSerilizationOptions is only created once

@Eneuman Eneuman requested review from a team, Tatsinnit and sabbour as code owners March 19, 2023 14:22
@Eneuman Eneuman changed the title WIP: Refactoring json serialization Refactoring Json serialization Mar 19, 2023
@Eneuman Eneuman changed the title Refactoring Json serialization WIP: Refactoring Json serialization Mar 19, 2023
@Eneuman Eneuman changed the title WIP: Refactoring Json serialization Refactoring Json serialization Mar 21, 2023
@hsubramanianaks
Copy link
Collaborator

Closing PR to kick in chatgpt review.

@Eneuman Eneuman mentioned this pull request Apr 24, 2023
Closed
@hsubramanianaks hsubramanianaks added the e2e testing e2e is in progress label Apr 25, 2023
hsubramanianaks
hsubramanianaks requested changes Apr 26, 2023
View reviewed changes
Copy link
Collaborator

@hsubramanianaks hsubramanianaks left a comment

Choose a reason for hiding this comment

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

@Eneuman overall looks good, I would suggest adding more UT's for Json serialize and deserialize for logging purpose both indented and non indented to cover different use case ex: like issue. seems like Live tests passed so we don't have to wait for testing team we can merge to main and they would test them in main. :)

hsubramanianaks
hsubramanianaks approved these changes May 22, 2023
View reviewed changes
Copy link
Collaborator

@hsubramanianaks hsubramanianaks left a comment

Choose a reason for hiding this comment

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

looks good, testing passed.

@cxznmhdcxz cxznmhdcxz merged commit 8edcb42 into Azure:main May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hsubramanianaks hsubramanianaks hsubramanianaks approved these changes

@Tatsinnit Tatsinnit Awaiting requested review from Tatsinnit Tatsinnit is a code owner

@sabbour sabbour Awaiting requested review from sabbour sabbour is a code owner

Assignees
No one assigned
Labels
e2e testing e2e is in progress review done
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Eneuman @hsubramanianaks @cxznmhdcxz