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

Drop get prefixes from almost all things #3332

Merged
merged 1 commit into from
Mar 21, 2021
Merged

Conversation

dualspiral
Copy link
Contributor

@dualspiral dualspiral commented Mar 11, 2021

SpongeAPI | Sponge

Hey look. An evening to myself!

As much as I dislike this, a decision has been made to drop the get prefix from our methods. So, I've done that for the command API - we need to get this ball rolling properly. I'd also like to resolve the question of builders before I pull this, but please let's not do that here, I'm really not interested in another flame war about whether it should be set or add or whatever - the leads can decide that.

I haven't done it for anything else because there may be instances where renaming has other unintentional side effects. See SpongeStringReader for an example here - some of the methods were shared with the Brig string reader and we're not transforming Brig. getRead had to become parsed (better name welcome). Thus, I think the best way to do this big rename is on a package basis where those who are in the know about their stuff does it.

I went and cobbled together an IntelliJ plugin to create an inspection and quick fix to help speed up the process of refactoring - https://github.com/dualspiral/drop-get-plugin - I'm not publishing it but if anyone else wants to use it/improve it, be my guest (or if we want to clean it up and pull it into the Sponge org, I don't mind).

@dualspiral
Copy link
Contributor Author

Okay - so I went a bit further and most gets are gone. There are 84 get methods left on:

ChannelBuf - they mirror Netty methods so it's not so simple to remove these
DataView, DirectionRelativeDataHolder, LocationBaseDataHolder and ValueContainer - these all have getInt etc. methods

getFinal has been renamed with appropriate names in transactions.
get has become find in the UserManager

I am sure there will be many AbstractMethodErrors and there are a lot of errors on running (it ended up crashing), but semi-automatic tools were only going to get us so far.

Errors can be found here

I have no more time tonight but others can work it this if they so wish.

@dualspiral dualspiral changed the title Drop get prefixes from all things command Drop get prefixes from almost all things Mar 18, 2021
@dualspiral
Copy link
Contributor Author

For what it's worth, I wrote a (pretty hacky) plugin to find API methods that weren't implemented in NMS classes, the results and the plugin are available here: https://gist.github.com/dualspiral/e539e37910b0a379aa3e42bb6a219e68

Nowhere near a perfect plugin, but it does pick out a few things I can go to fix as a result of this rename - as well as things that were never implemented.

@dualspiral
Copy link
Contributor Author

Well, based on the scans and such I've done, it looks like the methods that don't exist (per my plugin) are pretty much those that weren't there in the first place - things seem to not be implemented around data, inventory, entity and world stuff.

As of yet, I have not renamed is methods. There are potentially additional complications and considerations around them (doing something like isAnyOf(X...) to anyOf(X...) feels bad to me because the is indicated a true/false, without it do you just get any of X back?) - but it looks like there are less than 200 of them so it can be dealt with later - as opposed to the over 2000 get calls.

If anyone is mad enough to review, now's your chance. It might be worth others pulling this down and testing it themselves. Otherwise, minus the things I've not changed above, I think we should get this pulled in so that the last big break is behind us.

@dualspiral dualspiral merged commit 2d955d1 into api-8 Mar 21, 2021
@dualspiral dualspiral deleted the api8/command-forget branch March 21, 2021 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants