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

Switch to System.Text.Json #4002

Merged
merged 25 commits into from
Dec 21, 2023
Merged

Switch to System.Text.Json #4002

merged 25 commits into from
Dec 21, 2023

Conversation

LtRipley36706
Copy link
Member

Removed Newtonsoft.Json and DouglasCrockford.JsMin

Existing installs appear to be compatible.

Need to test compatibility with importing and exporting JSON files, "out of the box" setup, and verify changes do not break shared projects such as aclogview, ACViewer, GDLCacheBinParser to the extent that they use any of the shared common projects.

@LtRipley36706
Switch to System.Text.Json
678a856 
Removed Newtonsoft.Json and DouglasCrockford.JsMin
@LtRipley36706
Copy link
Member Author

Updated to confirm out of box setup and JSON exports now functional

@Mag-nus
Copy link
Member

Mag-nus commented May 8, 2023

I think we have the jsmin for the comment support in our config files

@LtRipley36706
Copy link
Member Author

LtRipley36706 commented May 9, 2023 •
edited
Loading

I think we have the jsmin for the comment support in our config files

Config = JsonSerializer.Deserialize<MasterConfiguration>(fileText, new JsonSerializerOptions { ReadCommentHandling = JsonCommentHandling.Skip, NumberHandling = JsonNumberHandling.AllowReadingFromString });

ReadCommentHandling = JsonCommentHandling.Skip is what DouglasCrockford.JsMin was doing.
NumberHandling = JsonNumberHandling.AllowReadingFromString resolves an issue found in a few places in our files where we put numbers inside quotes, but truly meant for them to numbers.

There was also an issue with our starterGear.json file that the boolean values were being placed inside quotes, so I had to write a custom converter to ensure compatibility

@LtRipley36706 LtRipley36706 marked this pull request as ready for review May 23, 2023 17:07
Mag-nus
Mag-nus reviewed Sep 16, 2023
View reviewed changes
Copy link
Member

@Mag-nus Mag-nus left a comment

Choose a reason for hiding this comment

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

I commented on the files that have host specific defaults, which I'm not to keen on. I think host specific properties should explicitly come from an external config file (like they do now)

I focused my review on ACE.Common and ACE.Server. The remaining changes in these two libraries looked fine.

What a massive PR. Bet it was a ton of work, but nice to see us moving to the new recommended native json support!

Source/ACE.Common/DatabaseConfiguration.cs
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I feel to keen about compiling host specific defaults into the binary.

Source/ACE.Common/GameConfiguration.cs
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I feel to keen about compiling host specific defaults into the binary.

Source/ACE.Common/MySqlConfiguration.cs
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I feel to keen about compiling host specific defaults into the binary.

Source/ACE.Common/NetworkSettings.cs
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I feel to keen about compiling host specific defaults into the binary.

@LtRipley36706
Copy link
Member Author

merged with latest and updated to reflect recent additions

Mag-nus
Mag-nus previously approved these changes Nov 26, 2023
View reviewed changes
Mag-nus
Mag-nus previously approved these changes Nov 28, 2023
View reviewed changes
@LtRipley36706 LtRipley36706 merged commit 12e5696 into ACEmulator:master Dec 21, 2023
2 checks passed
WarlockHolmez added a commit to daralet-ac/ACE that referenced this pull request Jan 17, 2024
@WarlockHolmez @aquafir
@dependabot @LtRipley36706 @gmriggs @Mag-nus
Update to ACE v1.57.4468 (#38)
159bf61 
* Prefer shared types in plugin loader (ACEmulator#4060)

* Bump Microsoft.EntityFrameworkCore from 6.0.24 to 6.0.25 in /Source (ACEmulator#4066)

* [ci skip] Updating ServerBuildInfo_Dynamic.cs

1.55.4454.20231128161918-master-ea48116

* fix appraisal vital colors for buff/debuff (ACEmulator#4067)

* add missing shield mastery to admin /buff command (ACEmulator#4068)

* [ci skip] Updating ServerBuildInfo_Dynamic.cs

1.55.4456.20231202215153-master-4868384

* [ci skip] Updating dbversion.txt

0.9.277

* Improve BiotaSQLWriter for ACLogView (ACEmulator#4071)

This also fixes the missing order column for BiotaPropertiesPalette

* [ci skip] Updating ServerBuildInfo_Dynamic.cs

1.55.4457.20231217201555-master-5eea515

* Bump MySqlConnector from 2.3.1 to 2.3.2 in /Source (ACEmulator#4072)

* player corpse FFA after first open (ACEmulator#4070)

* [ci skip] Updating ServerBuildInfo_Dynamic.cs

1.55.4459.20231221190151-master-a366be7

* Switch to System.Text.Json (ACEmulator#4002)

* Switch to System.Text.Json

Removed Newtonsoft.Json and DouglasCrockford.JsMin

* Update GameConfiguration.cs

* Update MasterConfiguration.cs

* Update ThreadConfiguration.cs

* Update StarterSpell.cs

* Update StarterGearFactory.cs

* Update Program_Setup.cs

* Update LifestonedConverter.cs

* Update Program_Setup.cs

* Update Program_Setup.cs

* Update ACE.Adaptor

Shift JSON weenie [de]serialization to System.Text.Json

* Update LifestonedConverter.cs

* Update StarterGearFactory.cs

* Update ACE.Server.csproj

* Update ACE.Server.Tests.csproj

* Update Program.cs

* Update MySqlConfiguration.cs

* Update DDDConfiguration.cs

* Update ModContainer.cs

* Update ModManager.cs

* Update ModMetadata.cs

* Update ModMetadata.cs

* [ci skip] Updating ServerBuildInfo_Dynamic.cs

1.55.4460.20231221191643-master-12e5696

* Update SphereTests.cs (ACEmulator#4073)

* [ci skip] Updating ServerBuildInfo_Dynamic.cs

1.55.4461.20231221193642-master-7eadaa5

* Update appveyor.yml

* [ci skip] Updating ServerBuildInfo_Dynamic.cs

1.56.4462.20231221194446-master-fd13097

* improving network resiliency (ACEmulator#4074)

* [ci skip] Updating ServerBuildInfo_Dynamic.cs

1.56.4463.20231225032139-master-5639fac

* refactoring some quest logic (ACEmulator#4077)

* [ci skip] Updating ServerBuildInfo_Dynamic.cs

1.56.4464.20231228135122-master-7785f44

* Fix issue with OOBE Setup (ACEmulator#4078)

* Bump MySqlConnector from 2.3.2 to 2.3.3 in /Source (ACEmulator#4076)

* [ci skip] Updating ServerBuildInfo_Dynamic.cs

1.56.4466.20231229174746-master-7dd702a

* Adjust Barbershop handling (ACEmulator#4079)

* Adjust Barbershop handling

* Delete Player_Barber.cs

* Update Player_Character.cs

* Update Player_Use.cs

* Update Player_Character.cs

* [ci skip] Updating ServerBuildInfo_Dynamic.cs

1.56.4467.20231231201925-master-e56390a

* Update appveyor.yml

* [ci skip] Updating ServerBuildInfo_Dynamic.cs

1.57.4468.20231231202603-master-4d73c70

* Bump Lib.Harmony in /Source (ACEmulator#4081)

* [ci skip] Updating ServerBuildInfo_Dynamic.cs

1.57.4469.20240102154739-master-16ea384

* Add AllowTrailingCommas to JsonSerializerOptions (ACEmulator#4082)

Additionally moved all serializers to single static instance in ACE.Common

* [ci skip] Updating ServerBuildInfo_Dynamic.cs

1.57.4470.20240102155446-master-0d6f9dd

* Switch from BCrypt.Net-Core to BCrypt.Net-Next (ACEmulator#4083)

BCrypt.Net-Core was a 3rd party release to port BCrypt.Net to .NET Core in the early days of .NET Core.

BCrypt.Net-Next is the current standard for BCrypt.Net
https://github.com/BcryptNet/bcrypt.net

* Use known UTC offset instead of TimeZoneConverter (3rd party) (ACEmulator#4084)

* Use System.TimeZoneInfo instead of TimeZoneConverter (3rd party)

* Don't rely on system lookup to find out EST is -5 UTC

* Remove some unreferenced System packages (ACEmulator#4085)

Not sure why these were ever added in the first place. They are not needed as nuget to build.

* [ci skip] Updating ServerBuildInfo_Dynamic.cs

1.57.4473.20240108115117-master-7aa5402

* Remove a few more system packages from the ARM64 checkin (ACEmulator#4086)

Just a few more packages that snuck in during the ARM64 docker support.

* [ci skip] Updating ServerBuildInfo_Dynamic.cs

1.57.4474.20240109004701-master-a1debb6

* Remove MySql.Data package ref (ACEmulator#4087)

This is the heaviest ref so it's nice to remove.

We used MySql.Data to execute database update scripts since this pr: https://github.com/ACEmulator/ACE/pull/2736/files

Now we use MysqlConnector

* [ci skip] Updating ServerBuildInfo_Dynamic.cs

1.57.4475.20240111005845-master-74b57b2

* Update Player_Inventory.cs

* chore: remove log4net

* Update NetworkSession.cs

* Update Program_BinUpdates.cs

* Update starterGear.json

---------

Co-authored-by: aquafir <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Ty Conner <[email protected]>
Co-authored-by: gmriggs <[email protected]>
Co-authored-by: Mag-nus <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mag-nus Mag-nus Mag-nus left review comments

@gmriggs gmriggs Awaiting requested review from gmriggs

@OptimShi OptimShi Awaiting requested review from OptimShi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@LtRipley36706 @Mag-nus