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

Newtonsoft.Json -> System.Text.Json #37

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

beppemarazzi
Copy link

@beppemarazzi beppemarazzi commented Nov 3, 2023

First quick and dirty attempt to migrate from Newtonsoft.Json to System.Text.Json
See #22 and datalust/seq-api#125

@nblumhardt WARNING: this PR is for your reference only: please review it carefully!!!!
All the tests on this repo are passing, but probably there are some API names are no more correct.
i.e.

public static LogEvent ReadFromJObject(JObject jObject)

now becomes

public static LogEvent ReadFromJObject(in JsonElement jObject)

but probably the name ReadFromJsonElement is more appropriated than ReadFromJObject

obviously there are also breaking changes when the public API references Newtonsoft.Json

@nblumhardt
Copy link
Member

This looks like a great start, thank you Giuseppe! I'll dig thoroughly into this and try to push it forward before moving over to the other PR.

We'll shortly be releasing a new version of Serilog.Formatting.Compact.Reader to support Serilog 3.1 (with the addition of built-in trace and span ids), so the best time to merge will be after the dust has settled on that one, in a month or so, I think.

@Numpsy
Copy link
Member

Numpsy commented Jun 16, 2024

Is this likely to go any further in the near future? (not sure how risky it is, just wondering in the context of a major version bump for Serilog 4)

@nblumhardt
Copy link
Member

I don't think this will be achievable in the post-Serilog-4 package rollout, needs more analysis.

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.

3 participants