Make json.net internal - Githubissues

exceptionless / Exceptionless.Net

Exceptionless clients for the .NET platform

https://exceptionless.com

Other

555 stars 142 forks source link

Make json.net internal #199

Closed yang-xiaodong closed 5 years ago

yang-xiaodong commented 5 years ago

Hello @ejsmith @niemyjski ,

I think we have all realized the problems(#165) about that the Json.NET is public. I also encountered these problems in my project. This PR is used to fix this problem.

This PR contains the following modifications:

Change Newtonsoft.Json to internal
Rename JsonIgnoreAttribute to DataMemberIgnoreAttribute
Make DataMemberIgnoreAttribute access level to public

Related issues : #165

CLAassistant commented 5 years ago

All committers have signed the CLA.

niemyjski commented 5 years ago

Hello,

I really appreciate these changes, can you please update the powershell script (https://github.com/exceptionless/Exceptionless.Net/blob/master/src/Exceptionless/update-json.ps1) to internalize it and rename this attribute. Without doing so makes it very very hard for us to upgrade son.net going forward.

ejsmith commented 5 years ago

This is a breaking change for anyone that makes use of those attributes. There is another Json attribute that lets you rename a member.

Why would we change the name of JsonIgnoreAttribute to DataMemberIgnore? DataMemberIgnore is a built in framework attribute name that we would be conflicting with. It seems like we could just rely on the fact that JsonNet honors the built in framework DataMember* attributes and just keep everything internal. People would need to use those attributes to control serialization. Only problem with that is that they may want to ignore something they are sending to exceptionless but not ignore it within their app.

yang-xiaodong commented 5 years ago

@niemyjski I am manually updating these files, I am a PowerShell noob, it is difficult for me to change the script.

yang-xiaodong commented 5 years ago

@ejsmith Sorry, I saw the suggestion of DataMemberIgnore in this issue comment. English is not my first language may be an understanding error. Do I need to change it back to JsonIgnoreAttribute and set it to public ?

niemyjski commented 5 years ago

Hello, yes that would be preferred just to make that public, also can we update the powershell script to make these changes so we can upgrade easily to newer versions of json.net .

ejsmith commented 5 years ago

I was thinking that we should rename them to ExceptionlessIgnoreAttribute. So you know that you are ignoring just for Exceptionless.

yang-xiaodong commented 5 years ago

@niemyjski Can I use Python script to replace PowerShell script?

niemyjski commented 5 years ago

We'd prefer to keep it powershell due to it runs cross plat also it already exists, we'd just need to update/add 2-3 replacement statements.

yang-xiaodong commented 5 years ago

@niemyjski I have updated the PowerShell script, and test passed on macOS.

niemyjski commented 5 years ago

Thanks for the updated powershell script! I agree with Eric on renaming the ignore attribute to ExceptionlessIgnoreAttribute Then you know you are specifically ignoring it only in exceptionless.

yang-xiaodong commented 5 years ago

I updated , please check.

By the way , the code in 9.0.1.zip is inconsistent with our current source code (Newtonsoft.Json), I have not replaced it.

yang-xiaodong commented 5 years ago

@ejsmith @niemyjski

niemyjski commented 5 years ago

Looks good thanks for the pr! I'm going to try updating us to the latest json.net and push a release soon. It would be nice to get some more changes in the next release.