davo899 / CobblemonTrainers

Adds a trainer battle system to Cobblemon with challenging trainer AI
Other
1 stars 4 forks source link

added option to set `moneyReward` integrating economy APIs #13

Closed KiwiFlavoredApollo closed 5 months ago

KiwiFlavoredApollo commented 5 months ago
  1. added command setmoneyreward.
  2. moneyReward option does not replace winCommand. Users can choose between the two whichever suits them better.

I am using EightsEconomyP mod which utilizes OctoEconomyAPI and confirmed working. However, CommonEconomyAPI still needs tests. CommonEconomyAPI seems promising but I couldn't find any economy mods that use the API.

Impactor is in my TODO list since Cobblemon GTS, Cobblemon Hunt and GUI Shop mods utilize Impactor for economy.

Like you mentioned in Discord server, this does not add new functionality. However, integrating economy APIs appeals to server admins who wants to unify economy system among mods. One can achieve the same goal with winCommand but moneyReward simplifies things just by specifying reward amount rather than typing the full command.

davo899 commented 5 months ago

I appreciate the work you've done, and your code is perfectly good, but your justification really doesn't make sense in my opinion. "Appeals to server owner who want to unify economy among mods", how is that not already possible? By allowing any command upon win, every economy mod is covered. "moneyReward simplifies things", I'd argue it makes them more complicated. Now every trainer has a new field, and theres 2 ways to make money rewards which could cause confusion between staff configuring trainers, and on top of that this mod needs to keep up to date with every economy API and add coverage for new economy mods. In my mind, adding this is a clear mistake.

KiwiFlavoredApollo commented 5 months ago

If you have made up your mind, then I won't try to persuade further as I understand your concern that giving two options for granting reward likely cause confusion among staffs using CobblemonTrainer. I agree that winCommand provides more universal way to give reward with a bit of limitation.

In my humble opinion, however, typing full winCommand every time I create a new trainer felt tedious and was prone to bugs due to typos. It's basically user's fault not using the mod correctly but I wished there were more foolproof way to specify reward.

davo899 commented 5 months ago

Well I had left it open just to see what you had to say that maybe I hadn't considered. I do understand the problem you are bringing up, but yeah having 2 ways to make the same reward is an issue. I do think it's fair to assume that every user of this mod will want to have trainers give money upon winning, but since people definitely do want the win command feature, I don't really know of a solution that adds economy integration without encountering the 2 ways problem again.

On the problem of accidentally typing win commands wrong, I think I might know a way to use Minecraft's command parser to prevent mistakes.

Also just want to say, the main rule I follow is that the code is the minimum code required for the target functionality. That keeps it as maintainable and scalable as possible. To be honest I should remove the moneyReward backwards compatibility now, as the server it was for has long since updated to new versions.