Skip to content

Conversation

@erikzhang
Copy link
Member

No description provided.

@superboyiii
Copy link
Member

@erikzhang Can you add some unit tests first. This is a large modification.

@cschuchardt88
Copy link
Member

Why we keep reinventing the wheel here? We should be using NewtonSoft.Json or something similar.

@Jim8y
Copy link
Contributor

Jim8y commented Jan 4, 2024

Why we keep reinventing the wheel here? We should be using NewtonSoft.Json or something similar.

For making the whole process controllable and determinastic.

@vncoelho
Copy link
Member

vncoelho commented Jan 4, 2024

As well as performance.
We did some experiments in the past.
Maybe nowadays it is faster but not sure.

@cschuchardt88
Copy link
Member

But we are using a stone wheel here on a spot car for instance.

@vncoelho we don't use huge json. So benchmarks will not show much change. But as for the useability and flexibility it's 1000 times better than what we have, and plus we can tie into other systems easily. It will help fix #3037 out of the box and also has what this PR is reinventing. So all the problems that you will eventually find or run into, are already done with any good json library on nuget like NewtonSoft.Json (157,196,338 downloads; that should tell you something)

@vncoelho
Copy link
Member

vncoelho commented Jan 4, 2024

I am not against it.
Perhaps it is worth give a try.

@Jim8y Jim8y added the Need Active Pr will be closed after one week if no new activity. label Feb 12, 2024
@Jim8y
Copy link
Contributor

Jim8y commented Feb 18, 2024

Close as inactive

@Jim8y Jim8y closed this Feb 18, 2024
@shargon shargon deleted the json-serialization branch February 18, 2024 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Need Active Pr will be closed after one week if no new activity.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants