Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[API 8] Resolve removal of get prefix on certain methods that didn't accept the change too nicely #2320

Closed
NickImpact opened this issue Mar 21, 2021 · 10 comments
Labels
api: 8 (u) version: 1.16 (unsupported since Oct 17th 2023) status: accepted

Comments

@NickImpact
Copy link

Aiming to use this issue as a general repository of method names in the API that, following the get prefix removal, received confusing renames.

Please append any issues you spot to this ticket rather than creating a new one, so we can have a global repository and one singular issue aimed at fixing the problem. Also, please provide possible renames of the methods so they can be properly discussed.


Known so far:

EconomyService

  • getOrCreateAccount --> orCreateAccount
    • accountOrCreate (zml) or createAccountIfAbsent (mbaxter)

Objective

  • getOrCreateScore --> orCreateScore
    • scoreOrCreate or createScoreIfAbsent
@limbo-app limbo-app added the status: needs triage This label is automatically applied to new issues and pull requests to indicate they require triage label Mar 21, 2021
@kashike kashike added api: 8 (u) version: 1.16 (unsupported since Oct 17th 2023) status: accepted labels Mar 21, 2021
@limbo-app limbo-app removed the status: needs triage This label is automatically applied to new issues and pull requests to indicate they require triage label Mar 21, 2021
@kashike
Copy link
Contributor

kashike commented Mar 21, 2021

e5717ac

@dualspiral
Copy link
Contributor

aee5e7c for economy service.

Will leave this issue open for the time being for any other weird results from dropping get.

@dualspiral
Copy link
Contributor

GameProfileCache now has methods such as byId, byName etc. I feel like these should be named find and overloaded if possible, or at least findById(...) etc. Thoughts?

@ImMorpheus
Copy link
Contributor

ImMorpheus commented Mar 22, 2021

GameProfileCache now has methods such as byId, byName etc. I feel like these should be named find and overloaded if possible, or at least findById(...) etc. Thoughts?

+1

Can we rename similar methods for consistency ?
StateContainer#statePropertyByName
Scheduler#tasksByName
Scheduler#tasksById
State#statePropertyByName

@dualspiral
Copy link
Contributor

We should - open for comments but I don't want to leave this proposal too long.

@zml2008
Copy link
Member

zml2008 commented Mar 23, 2021

For GameProfileCache, how about a scheme like withName or named?

@NickImpact
Copy link
Author

I'm more on the side of withX, as I can imagine id not coming across as straight as it should.

@dualspiral
Copy link
Contributor

dualspiral commented Mar 23, 2021

I'm not sure about with, as with is generally used to return an immutable copy of itself with a modified attribute.

Note that I have used find for the user manager - it's worth discussing that too as I'd argue is the same thing here. Also probably the same with account.

I've been thinking more about accountOrCreate and scoreOrCreate and I feel a little iffy about them because it doesn't seem natural to have nounOrVerb - I think that's why I went find in the first place. I think this is somewhere that we need to have a consistent policy.

@Zidane @gabizou any thoughts on how you'd like to handle this?

@dualspiral
Copy link
Contributor

We should - open for comments but I don't want to leave this proposal too long.

Famous last words.

I've spent some time thinking about this and the more I look at it, the more I really dislike nounOrVerb(...) methods - it's jarring. My preference remains to prefix these methods that grab a result from a collection or collection-like objects should be findX. It's what we have on other objects such as defaulted registries and such. This includes things that have been renamed as part of this issue already, scoreOrCreate (get a score or create?) should really be findOrCreateScore, for example. Similar case with the econ service. The methods should still make sense in English.

find would match the registry querying etc. and I think it makes the most sense here. We can omit the noun if the object's interface name makes it clear as to what you're getting (so find on UserManager makes sense without "User", but find on Objective doesn't make sense without Score as Objective#find() could be construed to get an objective and not a score towards the objective.)

I'll go ahead and use find and findOrCreate unless anyone has a good reason as to why we shouldn't use find in these scenarios.

@dualspiral
Copy link
Contributor

Nothing else has come up in the past month so I'll close this. It can be re-opened if more comes to light.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: 8 (u) version: 1.16 (unsupported since Oct 17th 2023) status: accepted
Projects
No open projects
Development

No branches or pull requests

6 participants