Tradeshop / TradeShop

Unique, new, powerful Minecraft TradeShop plugin!
https://tradeshop.github.io
Apache License 2.0
25 stars 11 forks source link

Removed unused (de)serialization methodes #147

Open BoyStijn opened 1 year ago

BoyStijn commented 1 year ago

Pull Request Etiquette

There are several guidelines you should follow in order for your Pull Request to be merged. Replace [ ] with [x] to cross the checkbox.

It is sometimes better to include more related changes in a single commit. If you find yourself having an overwhelming amount of commits, you can rebase your branch.

Changes

Removed unused (de)serialization methodes

KillerOfPie commented 1 year ago

Were these changes tested? Removing serializable and not replacing it with something else is a significant change and needs to be reasonably tested to make sure that the data output isn't changing/breaking shops that previously existed.

BoyStijn commented 1 year ago

Yes i have ran it on a new paper server. The config and Data is both saved and loaded properly. I already mentioned in issue #145 that if you want custom JSON de(serialization) you'll have to implement the com.google.gson.JsonDeserializer<T> and com.google.gson.JsonSerializer<T> interfaces

KillerOfPie commented 1 year ago

Yes i have ran it on a new paper server. The config and Data is both saved and loaded properly. I already mentioned in issue #145 that if you want custom JSON de(serialization) you'll have to implement the com.google.gson.JsonDeserializer<T> and com.google.gson.JsonSerializer<T> interfaces

Update this with 2.6 since I had to make an adapter for itemstacks.

As long as it works without the serializable extensions then I'm fine with those being removed.

Changing (de)serializable methods feels unnecessary and even if they aren't being used by gson they roll in and out of usage for debugging data output. toString should focus more on a readable format for debugging(with an exception for smaller objects that are easily readable in their serialized form in which case it can redirect to that)

I didn't make any of these changes myself to avoid stealing credit for your work/testing.

BoyStijn commented 1 year ago

i wont be able to update it today, ill probably have it finished tomorrow. I might consider making proper Serialization classes if that something you're interested in

KillerOfPie commented 1 year ago

i wont be able to update it today, ill probably have it finished tomorrow. I might consider making proper Serialization classes if that something you're interested in

That's fine, it's not looking like I'll get a release out before the end of the week anyways.

I already added on for Configuration Serializable. I don't think any of our stuff needs it since the normal gson handles it pretty well. If you really want to it's fine as long as the output and input don't change, I just don't see any benefits that would justify the work needed for it.

BoyStijn commented 1 year ago

Merge Conflicts have been resolved. Tested on 1.19.2. Maybe do another test yourself, but i would say you're ready for release with 1.19 support