ArcanePlugins / Treasury

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

First preparations for event calling #164

Closed yannicklamprecht closed 2 years ago

yannicklamprecht commented 2 years ago

Implements Event calling.

TODOS:

yannicklamprecht commented 2 years ago

A lot of events aren't defined yet, so it will have some some todos. Feel free to discuss which events are needed.

Probably Missing Events?!:

lokka30 commented 2 years ago

Thank you very much! I have a few questions:

And here are my opinions on each event you listed above:

[View Opinions...] - I would prefer using the word `Update` than `Change` and `Modify` in the event names. - Unsure if I prefer present/post-tense words (e.g. `do-transaction` and `did-transaction`) or using prefixes like `Pre` and `Post` (e.g. `pre-transaction` and `post-transaction`). - I'm unsure about any of these events being modifiable / cancellable. It could break plugins by cancelling them or changing their state. Yes: - `AccountBalanceModifyEvent` - `AccountBalanceModifiedEvent` - `AccountDoTransactionEvent` - `AccountDidTransactionEvent` - `AccountDeleteEvent` - `AccountDeletedEvent` - `AccountModifyPermissionEvent` - `AccountModifiedPermissionEvent` Maybe: - `AccountNameChangeEvent` - `AccountNameChangedEvent` - `AccountRetrieveBalanceEvent` - `AccountRetrievedBalanceEvent` - `AccountHistoryRetrieveEvent` - `AccountHistoryRetrievedEvent` - `AccountMemberIdsRetrievedEvent` - `AccountIsMemberEvent` - `AccountAddMemberEvent` and `AccountRemoveMemberEvent` - added these two to the list, thought they were missing from the original - `AccountPermissionsRetrieveEvent` - `AccountPermissionsRetrievedEvent` Probably no: - `PlayerAccountBalanceResetEvent` - `AccountPermissionCheckEvent` - `AccountPermissionCheckedEvent`
MrIvanPlays commented 2 years ago

as i see it, this api you will introduce will be a wrapper of the implemented api by economy providers, am i right? if i am, i don't really like this approach, but if others are agreeing i won't stop from merging.

lokka30 commented 2 years ago

as i see it, this api you will introduce will be a wrapper of the implemented api by economy providers, am i right? if i am, i don't really like this approach, but if others are agreeing i won't stop from merging.

I have the same opinion if such is the case.

yannicklamprecht commented 2 years ago

Feel free to close. 👍🏼 Yeah it's a wrapper. The alternative could be the Template Pattern. https://www.tutorialspoint.com/design_pattern/template_pattern.htm

lokka30 commented 2 years ago

Feel free to close. 👍🏼 Yeah it's a wrapper. The alternative could be the Template Pattern. https://www.tutorialspoint.com/design_pattern/template_pattern.htm

The template pattern sounds like a brilliant alternative! @MrIvanPlays do you think that route would be viable for this?

MrIvanPlays commented 2 years ago

that's gonna rock the boat too much. in which way we are obliged to call them? fgs they're only 2 events to call! the economy provider can do this easily.

lokka30 commented 2 years ago

that's gonna rock the boat too much. in which way we are obliged to call them? fgs they're only 2 events to call! the economy provider can do this easily.

Although I don't think it is at all necessary, if we do go with the template pattern then we can add several other events (see my original reply with the events list in a dropdown).

However I am currently more orientated to keep things the way it is.

lokka30 commented 2 years ago

Regardless of the PR being closed I am very grateful for your contribution @yannicklamprecht. Thank you very much. :)