Add specific methods for handling non-player accounts

[lokka30]

See the post below, the 'Different Approach' part.

View initial post anyways

This has been very briefly discussed in the past. The opposition argument was that 'the method name was too long'. I am not satisfied with that, it is better to have a slightly long method name than to have to check the documentation constantly out of confusion. This change should not break anything as we can deprecate the old 'xyzAccount' methods and remove them in Treasury v2. Those methods can be `default`ed to just redirect to the new methods.

[lokka30]

Interested to hear the opinions of: @MrIvanPlays @Jikoo @MrNemo64 and anyone else that is interested - if possible. :)

[MrIvanPlays]

1. No
2. Too late to change stuff. Proposed deprecation way is not suitable at all. You're making Treasury unstable.

[MrNemo64]

At this point it's an unnecessary change I think

[lokka30]

If we leave it this way then the library will be forever plagued with it. Economy provider developers would be happy to make a simple change which only benefits them anyways.

We are renaming a method, not changing any parameters or return types. Modifying existing methods should be avoided at all costs of course, however, this change is very easy for the three or so economy provider developers out there.

I can’t see how my way of executing this change breaks anything (using the default deprecate system). We are adding methods and marking an old ones for removal. Nothing breaks.

I know this makes Treasury less stable. But I strongly believe this needs to be done right now.

[MrIvanPlays]

So we're not focusing stability but "Economy provider developers would be happy". If you want to make "Economy provider developers" "happy" then break all the API how they want it (which is basically vault 2.0, what you don't want, or do you?) and suddenly Treasury's reputation now features "An unstable API". Do you want this to happen? If you do, then I'm outta here.

[lokka30]

I think you are exaggerating how much affect this will have. We give very ample time for developers to adapt to API changes. With such little adoption currently in place, this idea is not out of this world to go forward with. We are in the ONLY time frame ever where we are able to make this change without too much negative effect. Either we do it now or we don’t.

Democracy shall decide and see how we have to cope with our decisions in the future. Maybe this affects Treasury in the future, maybe it doesn’t, but I don’t like taking chances.

[MrIvanPlays]

I don't care how much people are currently using Treasury, be it 3 developers, or 100, API standards are API standards. You can't just go and poke with a stick and say "this is not right, we shall change it to <insert idea here>". If I say "I agree", then in the future when Treasury has more adoption you can say the same for something else. What do we do then? Shall I come and poke you with a stick and tell you "we shall swap your brain with einstein's because you're stupid"?

HARDLY against this change.

[lokka30]

I still vote yes but I won’t die on this hill. @MrNemo64 are you still voting no?

[MrNemo64]

I still vote yes but I won’t die on this hill. @MrNemo64 are you still voting no?

I don't like the idea of deprecating the methods. I see why you want to do this change but since the return type already says the account type that is returned you already know what is returned. Having new default methods that call the old ones is something I can see as a possibility but wouldn't make much sense. Also what methods would be affected by this?

[Jikoo]

I'm with MrNemo64, I get why you'd want to do this for consistency but I believe the return type is sufficiently clear. It's a lot easier to not rock the boat at this point - having 2.0 be minimally breaking is preferable.

[lokka30]

View outdated part of this comment

> > I still vote yes but I won’t die on this hill. > > @MrNemo64 are you still voting no? > > I don't like the idea of deprecating the methods. I see why you want to do this change but since the return type already says the account type that is returned you already know what is returned. Having new default methods that call the old ones is something I can see as a possibility but wouldn't make much sense. Also what methods would be affected by this? # Current plan: The current 'xyzAccount' methods which reference non-player accounts will be `@Deprecated` and `default`ed, so any new implementations don't use it and it just 'redirects' the method call to the new method. # Affected methods: * [EconomyProvider#hasAccount](https://github.com/lokka30/Treasury/blob/master/api/src/main/java/me/lokka30/treasury/api/economy/EconomyProvider.java#L130) -> `retrieveNonPlayerAccount` * [EconomyProvider#retrieveAccount](https://github.com/lokka30/Treasury/blob/master/api/src/main/java/me/lokka30/treasury/api/economy/EconomyProvider.java#L143) -> `retrieveNonPlayerAccount` * [EconomyProvider#createAccount](https://github.com/lokka30/Treasury/blob/master/api/src/main/java/me/lokka30/treasury/api/economy/EconomyProvider.java#L158) (including [this](https://github.com/lokka30/Treasury/blob/master/api/src/main/java/me/lokka30/treasury/api/economy/EconomyProvider.java#L158) of course) -> `createNonPlayerAccount` * **NOT** [EconomyProvider#retrieveAccountIds](https://github.com/lokka30/Treasury/blob/master/api/src/main/java/me/lokka30/treasury/api/economy/EconomyProvider.java#L189) * [EconomyProvider#retrieveAllAccountsPlayerHasPermission](https://github.com/lokka30/Treasury/blob/master/api/src/main/java/me/lokka30/treasury/api/economy/EconomyProvider.java#L269) -> `retrieveNonPlayerAccountsWithMembership`

Different approach? 🤔

I have also just thought of something that might be more suitable. We add the non-player specific methods, but also keep the 'general' non-specific 'account' methods, instead of removing those. Nothing in the API changes except for some additions and we will definitely add more information to the javadocs on the 'general account' methods so it is clearer to use them. @MrIvanPlays @MrNemo64 do you approve that idea instead, or still no good?

[lokka30]

Reasonable viewpoints. @Jikoo did you see my reply, specifically the 'different approach' part? I think I sent it the same minute you sent your reply, just making sure you saw it. 😄

[Jikoo]

Yeah, sorry, going through my notifications, I've been largely absent for about a week and they've built up more than I'd like.

I like the idea of being able to actually specify creation of a non-player account, that seems good. Useful for plugins like Towny where non-player accounts are identified by UUID and an economy provider might decide UUID string -> player account. Much more consistent.

[MrIvanPlays]

Can't understand. @Jikoo yes or no?

[Jikoo]

Yes to different approach. No to initial full rename, but not a hard no.

[MrIvanPlays]

I don't understand that different approach, can you explain for dumb @lokka30

[MrNemo64]

I actually see potential in the different aproach

[lokka30]

I don't understand that different approach, can you explain for dumb @lokka30

The 'different approach', which I actually now prefer over my initial idea of deprecation, is as follows:

• no existing methods are removed or deprecated or changed at all, except that their javadocs are edited so they are more concise of what their uses are.
• adding non-player methods so that there is no ambiguity on which account type is intended
  • e.g. createNonPlayerAccount exists alongside createPlayerAccount and createAccount
  • In this case, createAccount will be documented to have the following logic:
    • Creates an account with the specified ID.
    • If the ID is a valid UUID, AND if a player has joined the server with that UUID before, THEN it will create a player account.
    • Otherwise it will create a non-player account.
  • and other ambiguous account methods will be documented in a way which heavily suggests specific logic so that there is consistency across implementations.

Actually, this does bring the issue up that economy providers need to add a method that didn't previously exist. but I think this is better than modifying existing methods anyways. Unsure what version we should target for this, if we all agree with it in the first place. Maybe this is a good fit for v2 as we will also be removing the API version system in that version.

[MrIvanPlays]

I agree to be in v2.

[DeathGOD7]

Well having migrated as lokka30 suggested would be useful. As some people will easily understand it by just the method name. Also, it is about making easy-to-understand API rather than confusing API (especially names).

And if we are talking about changing some methods in economy provider then please also look into changing "retrieveAccountIds" to "retrieveAllAccountIds" to remove confusion.

Regards, Death GOD 7

[lokka30]

@DeathGOD7, thanks for the comment :)

Regarding the last part of your comment, I think the agreement so far has been that zero existing methods will change apart from adding constrictions to their documentation to ensure they will be used correctly. I think adding All would also unnecessarily lengthen the method names, especially when considering the little improvement it could yield compared to the existing integrations it would break and instability it would create.

Our current plan is to add new methods for non-players specifically, and that's it ;)

[DeathGOD7]

If the previous 'getAccount' is gonna be changed or migrated to 'getNonPlayerAccount' then not total necessary to change 'retrieveAccountIds' as it will be solo distinct from others methods.

[lokka30]

If the previous 'getAccount' is gonna be changed or migrated to 'getNonPlayerAccount' then not total necessary to change 'retrieveAccountIds' as it will be solo distinct from others methods.

I'm unsure what you mean, but just to clear things up, here is an example:

Currently there is retrievePlayerAccountIds and retrieveAccountIds.

Once this issue is solved there will also be retrieveNonPlayerAccountIds.

All this issue does is add non-player-specific methods. This makes the API easier to use and understand.

[DeathGOD7]

Ohh, its like slow migration to prevent API breaking changes got it.

On Sun, Mar 20, 2022 at 20:37 lach @.***> wrote:

If the previous 'getAccount' is gonna be changed or migrated to 'getNonPlayerAccount' then not total necessary to change 'retrieveAccountIds' as it will be solo distinct from others methods.

I'm unsure what you mean, but just to clear things up, here is an example:

Currently there is retrievePlayerAccountIds and retrieveAccountIds.

Once this issue is solved there will also be retrieveNonPlayerIds.

All this issue does is add non-player-specific methods. This makes the API easier to use and understand.

— Reply to this email directly, view it on GitHub https://github.com/lokka30/Treasury/issues/167#issuecomment-1073267475, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIV4GRZWDQXQJXX7NOUAN2DVA43SLANCNFSM5QOUM5YQ . You are receiving this because you were mentioned.Message ID: @.***>

[lokka30]

Ohh, its like slow migration to prevent API breaking changes got it. … On Sun, Mar 20, 2022 at 20:37 lach @.> wrote: If the previous 'getAccount' is gonna be changed or migrated to 'getNonPlayerAccount' then not total necessary to change 'retrieveAccountIds' as it will be solo distinct from others methods. I'm unsure what you mean, but just to clear things up, here is an example: Currently there is retrievePlayerAccountIds and retrieveAccountIds. Once this issue is solved there will also be retrieveNonPlayerIds. All this issue does is add non-player-specific methods. This makes the API easier to use and understand. — Reply to this email directly, view it on GitHub <#167 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AIV4GRZWDQXQJXX7NOUAN2DVA43SLANCNFSM5QOUM5YQ . You are receiving this because you were mentioned.Message ID: @.>

Nothing to do with speed :) we're just delaying this until v2 which should not take long to come out actually, maybe a month.

[lokka30]

Just took a look at this again. I'm unsure what the point of having the non-account-type-specific methods such as createAccount is. If people want to create a player account, they will specify that, same with a non-player account. Zero point in having ambiguity. No worries of breaking things if it is going in v2 since other things are being broken already, need to get them all out of the way whilst we can.

[MrNemo64]

Just took a look at this again. I'm unsure what the point of having the non-account-type-specific methods such as createAccount is. If people want to create a player account, they will specify that, same with a non-player account. Zero point in having ambiguity. No worries of breaking things if it is going in v2 since other things are being broken already, need to get them all out of the way whilst we can.

If we are already going to break thinks, better to break now. I also prefer not having createAccount

[MrIvanPlays]

My idea is to remove createPlayer and createNonPlayer and combine them into createAccount(..., AccountType)