ArcanePlugins / Treasury

🏦 A powerful multi-platform library for next-level plugin integrations.
https://hangar.papermc.io/ArcanePlugins/Treasury
Other
55 stars 14 forks source link

Replace EconomyTransactionInitiator with Cause #270

Closed MrIvanPlays closed 1 year ago

MrIvanPlays commented 1 year ago

I think this is needed as even though I am the one who created the EconomyTransactionInitiator I was confused by it. The new Cause interface is abstracted out from the economy api, which allows it's usage into other future apis (permissions api for example), and it has been made easier to use, with also adding one more type of initiators: accounts.

Before:

EconomyTransactionInitiator<UUID> initiator = EconomyTransactionInitiator.createInitiator(Type.PLAYER, playerUuid);
// ...

After:

Cause.Player cause = Cause.player(playerUuid);
// ...

This also allows to have more OOP than previously in this section, whilst also having a future-proof interface where if we need a new cause we don't need to break stuff.

MrIvanPlays commented 1 year ago

I've decided to simplify it by making every PlayerAccount and NonPlayerAccount a Cause. @Jikoo if you'd like you can do another review here.

MrIvanPlays commented 1 year ago

Waiting on @lokka30

lokka30 commented 1 year ago

This is an excellent idea with equally great execution.

One thing I would like clarified is Cause#account's name, I would expect a method of this name to return something that is for generic Accounts however it is intended for Non-Player accounts.

Should we rename account() to nonPlayer() so it's a little more consistent with player()? Note that it won't be fully consistent since player references a UUID rather than a PlayerAccount.

Cheers. This is an excellent improvement to the API.