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

⚠️ BREAKING CHANGES ⚠️ - Relocation of important API classes. #3139

Merged
merged 39 commits into from
Sep 3, 2021

Conversation

TheBusyBiscuit
Copy link
Member

@TheBusyBiscuit TheBusyBiscuit commented Jun 26, 2021

🌐 Description

This Pull Request introduces major changes which will break almost every addon in existence.
But before you panic, do not worry. This Pull Request will not be merged anytime soon.
Please read carefully.

1. Why... why do you do this to us? 😢

As you know, Slimefun is an ever-growing, ever-expanding project.
It has grown steadily since over 8 years and is constantly being rewritten over and over again.
With so many people working on the project, mistakes happen.
Slimefun has a reputation and bad code practices is a part of that.
We have made many bad design decisions over these 8 years, though frankly which developer hasn't over such a long time period.

We are also constantly working on improving the codebase and getting rid of these bad decisions by refactoring the codebase.
And we almost did our best to avoid breaking changes within the many dependents of the project, most noteworthy the many addons that exist for Slimefun.
However sometimes we find ourselves in a corner. A corner from which we cannot escape without breaking changes.
And we have reached many corners like that.

2. What has changed? 😨

I will try my best to sum up all the things that have changed within the API, so you can prepare accordingly.
We changed our package signature from me.mrCookieSlime.Slimefun to io.github.thebusybiscuit.slimefun4 a while ago.
But there are still a lot of classes who have missed the move, because they would break the API.

  1. Category has been renamed to ItemGroup.
  2. All Category / ItemGroup variants have been relocated to io.github.thebusybiscuit.slimefun4.api.items.groups
  3. The SlimefunItem class has been relocated to io.github.thebusybiscuit.slimefun4.api.items
  4. The SlimefunItemStack class has been relocated to io.github.thebusybiscuit.slimefun4.api.items
  5. The ItemHandler class has been relocated to io.github.thebusybiscuit.slimefun4.api.items
  6. The RecipeType class has been relocated to io.github.thebusybiscuit.slimefun4.api.recipes
  7. Research classes have been moved from io.github.thebusybiscuit.slimefun4.core.researching to io.github.thebusybiscuit.slimefun4.api.researches
  8. The main class SlimefunPlugin was renamed to Slimefun
  9. CS-CoreLib2 was removed and replaced by dough

And lastly, any method signatures that involved the aforementioned classes was also updated accordingly.

3. Will this open up any new possibilities? 🤞🏻

Not immediately.
But it helps us get long-overdue changes done.
This way, we can safely work on all the other API changes we are working on.
The following things are still to come:

  • Rewrite of the recipe system
  • Rewrite of the Programmable androids
  • Rewrite of the Inventory system
  • Rewrite of the Research system
  • Rewrite of our BlockStorage system

We believe these changes can be done without much breakage, but getting these bunch of design mistakes out of the way, let's us focus on the more important rewrites again.

4. Why do you not merge this anyway? 😈

Nooooooooooooo.
We aren't that evil - mostly.

We would also like to take this opportunity to be transparent about these changes and ask you and all addon developers out there.
We want to know:
a) When is the best time to merge these, so you can prepare.
b) Any other changes you want us to do now?

The second question might seem weird at first. But this large refactoring is also the perfect oppurtunity for change.
Right now, you have complete creative freedom over how the API's future will change.
Changes we would normally reject because they could break something, can now be integrated (maybe).

So please, engage in conversations below this pull request. Tell us your thoughts, opinions and suggest other changes we should make.

5. What can I do to prepare my addon? 😕

Once a date has been settled, make sure to be ready to update as soon as possible.
In the meantime, you can compile the breaking-changes/class-relocation branch of Slimefun4 and see how your addon will break, so you are prepared.

Also make sure that your Auto-Updater works and that your plugin does not get shutdown when Slimefun's api breaks.
By ensuring that the Auto-Updater still works, you can push updates without servers needing to update your addon manually to fix the issue.
It may not be the best practice but using a try/catch here and simply logging the exception is an option.
This way, the Auto-Updater can still do its thing, even if no item could be loaded.

@github-actions

This comment has been minimized.

@TheBusyBiscuit TheBusyBiscuit added the 🔧 API This Pull Request introduces API changes. label Jun 26, 2021
@WalshyDev
Copy link
Member

getByID still needs to be renamed to getById 👀
Now is a good time

@J3fftw1
Copy link
Contributor

J3fftw1 commented Jun 26, 2021

Whats the reason to call it a item group instead of a category. A category makes more sense right?

@TheBusyBiscuit
Copy link
Member Author

Whats the reason to call it a item group instead of a category. A category makes more sense right?

Category is already widely used, even within Java there is already java.util.locale.Category.
The name category also doesn't imply much besides... "categorization", which a Slimefun category not necessarily does.
The items aren't always grouped by concrete criterias, mostly just by which addon provided them.

ItemGroup is much less ambigious in my opinion, it groups items, simple as that.
And no class by that name exists at the moment.

@J3fftw1
Copy link
Contributor

J3fftw1 commented Jun 26, 2021

Whats the reason to call it a item group instead of a category. A category makes more sense right?

Category is already widely used, even within Java there is already java.util.locale.Category.
The name category also doesn't imply much besides... "categorization", which a Slimefun category not necessarily does.

Fair enough

The items aren't always grouped by concrete criterias, mostly just by which addon provided them.

ItemGroup is much less ambigious in my opinion, it groups items, simple as that.
And no class by that name exists at the moment.

I do still think calling it categories would be better since 1 addon is a category right within slimefun. ItemGroup just sounds weird IMO

@WalshyDev
Copy link
Member

image
Pssh, easy review

@Sefiraat
Copy link
Member

Not sure of the timeframes involved here, but if 24th July - 2nd Aug could be avoided, i'd appreciate it as i'll be in sunny Cornwall 🌞

@J3fftw1
Copy link
Contributor

J3fftw1 commented Jun 28, 2021

I wont be home for most of august

@Mooy1
Copy link
Contributor

Mooy1 commented Jun 28, 2021

Anytime after July 9th is good for me

@Seggan
Copy link
Contributor

Seggan commented Jun 30, 2021

I won't be on the computer much before July 20

@TheBusyBiscuit
Copy link
Member Author

I will use this to test our new sonarcloud workflow, don't mind me 👀

@TheBusyBiscuit
Copy link
Member Author

@here (wish I could ping this way...)

So @Sefiraat made the brilliant suggestion of pausing TheBusyBiscuit/builds for a short period of time when the update is supposed to go live.
This way, everyone will get a bit of time to update their addons and the next auto-update cycle should not only update to the latest Slimefun build but also to the latest build of every addon (which is hosted on the builds page).

I decided to extend this to a full weekend, that should be plenty of time for everyone to update.
So my proposal is the following: The builds page will be frozen on Friday, September 3rd 2021 and this pull request will be merged into master and stable simultaneously. RC-26 will be released and available on jitpack.io

The builds page will be un-frozen on Monday, September 6th 2021 and all updates will go live.
Is everyone okay with this update taking place on September 3rd - 6th 2021?

@WalshyDev
Copy link
Member

Sounds good to me

breaking-changes/class-relocation

Conflicts:
	src/main/java/io/github/thebusybiscuit/slimefun4/implementation/items/LimitedUseItem.java
	src/main/java/io/github/thebusybiscuit/slimefun4/implementation/items/androids/ProgrammableAndroid.java
	src/main/java/io/github/thebusybiscuit/slimefun4/implementation/items/electric/machines/ElectricSmeltery.java
	src/main/java/io/github/thebusybiscuit/slimefun4/implementation/items/electric/machines/enchanting/AutoDisenchanter.java
	src/main/java/io/github/thebusybiscuit/slimefun4/implementation/items/electric/reactors/Reactor.java
	src/main/java/io/github/thebusybiscuit/slimefun4/implementation/items/geo/GEOMiner.java
	src/main/java/io/github/thebusybiscuit/slimefun4/implementation/items/multiblocks/miner/IndustrialMiner.java
	src/main/java/io/github/thebusybiscuit/slimefun4/implementation/listeners/EnhancedFurnaceListener.java
	src/main/java/io/github/thebusybiscuit/slimefun4/integrations/IntegrationsManager.java
	src/main/java/io/github/thebusybiscuit/slimefun4/integrations/McMMOIntegration.java
	src/main/java/io/github/thebusybiscuit/slimefun4/integrations/OrebfuscatorIntegration.java
@TheBusyBiscuit
Copy link
Member Author

One month left. @WalshyDev already marked this pull request as ready for review.
Aside from upcoming merge conflicts, this should be feature-complete. So some testing of this would be nice @Slimefun/code-reviewers @Slimefun/bug-testers

@Mooy1
Copy link
Contributor

Mooy1 commented Aug 7, 2021

Will the inventory system rewrite be included in this?

@TheBusyBiscuit
Copy link
Member Author

Will the inventory system rewrite be included in this?

No

@TheBusyBot TheBusyBot added the ⚡ Merge Conflicts This Pull Request has merged conflicts which need to be resolved! label Aug 17, 2021
@TheBusyBiscuit TheBusyBiscuit removed the ⚡ Merge Conflicts This Pull Request has merged conflicts which need to be resolved! label Sep 1, 2021
@TheBusyBot TheBusyBot added the ⚡ Merge Conflicts This Pull Request has merged conflicts which need to be resolved! label Sep 2, 2021
@WalshyDev WalshyDev removed the ⚡ Merge Conflicts This Pull Request has merged conflicts which need to be resolved! label Sep 2, 2021
@WalshyDev
Copy link
Member

Alrighty, no more PR merges until we get this merged. This is gonna take a little bit to review but luckily enough it's still simple. Just large.

@TheBusyBot TheBusyBot added the ⚡ Merge Conflicts This Pull Request has merged conflicts which need to be resolved! label Sep 2, 2021
@TheBusyBiscuit TheBusyBiscuit removed the ⚡ Merge Conflicts This Pull Request has merged conflicts which need to be resolved! label Sep 2, 2021
@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 2, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug C 1 Bug
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 131 Code Smells

40.9% 40.9% Coverage
0.2% 0.2% Duplication

@TheBusyBiscuit
Copy link
Member Author

Here goes nothing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔧 API This Pull Request introduces API changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants