search

MilkBowl / VaultAPI

API Component of Vault
GNU Lesser General Public License v3.0
274 stars 112 forks source link

Multicurrency, UUID && legacy support #152

Closed anjoismysign closed 1 year ago

anjoismysign commented 1 year ago

It's a really long story... I first wanted to pull request multicurrency support but somehow I figure out that the project seemed abandoned but I still didn't want to deprecate software using original VaultAPI. So what I did is fork it and attempt to go with VaultAPI2 and Vault2. The idea behind VaultAPI2 is that there are now 3 layers of economy. The most basic one of it is the one that we have always used. Then we got the second layer which I called IdentityEconomy since it identifies players by their UUID with the mission of improving performance (most code of this second layer was taken from @LlmDl 's pull request). The last layer is a multicurrency abstraction. The idea of currencies is given as implementations which instead of creating a new object being specifically oriented to currencies, I figured that IdentityEconomy already ships with them.

I made an awesome (imo) example of how to always depend in MultiCurrency, even when it's not provided thanks to using currencies as IdentityEconomy. Now, in Vault2, I made it so all legacy Economy providers whenever attempting to register, Vault2 will also provide their IdentityEconomy layer so in case a plugin really only wishes to use IdentityEconomy, we saved them work 👍🏻 I need to mention that today, March 31st, I've been testing all this pull request with Vault 1.7.3 jar and there are no issues, it even works the implementation I previously mentioned (ElasticEconomy).

In case you wish to see how you can always depend in MultiCurrency even if it's never provided: https://github.com/anjoismysign/BlobLib/commit/68116fbdd30d437f5e86c14584064d6492b8d89a In case you are interested in merging Vault2: https://github.com/MilkBowl/Vault/compare/master...anjoismysign:Vault2:master

LlmDl commented 1 year ago

The VaultAPI project doesn't have a Contributing.md but I would never the less not accept this PR purely based on the unneeded whitespace diffs and the changes made to the pom.xml.

On a separate level: my entire PR was to get Vault off of using OfflinePlayers/Players for Economy accounts, but you have them included still.

anjoismysign commented 1 year ago

You are right, I forgot to revert the pom to its origin. Regarding removing OfflinePlayer, this would force old economy plugins to update... I completely agree with your proposal. I just disagree about leaving out software that already works...

I am going to revert that. I am really sorry for this issue, I submitted this at 2 am and I feel embarrassed for that mistake...

anjoismysign commented 1 year ago

What do you mean by unneeded whitespace diffs? Regarding your proposal, it's only an interface. There's no actual logic in it... I cannot figure out a way to explain in a better way how your interface works

LlmDl commented 1 year ago

Regarding removing OfflinePlayer, this would force old economy plugins to update... I completely agree with your proposal. I just disagree about leaving out software that already works...

If you followed the development of my pullrequest you would note that Vault2 was made with the conscious choice to have the old methods in the old package and the new methods in the vault2 package. When UUIDs are available there's no good reason to have potentially laggy OfflinePlayer methods involved.

anjoismysign commented 1 year ago

Ok, brb