Slimefun / Slimefun4

Slimefun 4 - A unique Spigot/Paper plugin that looks and feels like a modpack. We've been giving you backpacks, jetpacks, reactors and much more since 2013.
GNU General Public License v3.0
961 stars 546 forks source link

Recipe Rewrite #4078

Closed SchnTgaiSpock closed 8 months ago

SchnTgaiSpock commented 8 months ago

Description

Hey guys, this is my attempt at the recipe rewrite.

I have written up an ADR that goes over how the new API currently works, but in short:

Any feedback on the API or discussion around the new recipe system in general would be greatly appreciated!

Proposed changes

Related Issues (if applicable)

N/A

Checklist

github-actions[bot] commented 8 months ago

Your Pull Request was automatically labelled as: "🔧 API" Thank you for contributing to this project! ❤️

github-actions[bot] commented 8 months ago

Slimefun preview build

A Slimefun preview build is available for testing! Commit: 0a6c1b39

https://preview-builds.walshy.dev/download/Slimefun/4078/0a6c1b39

Note: This is not a supported build and is only here for the purposes of testing. Do not run this on a live server and do not report bugs anywhere but this PR!

SchnTgaiSpock commented 8 months ago

It's still a draft for now, since i haven't moved the workstations over to the new system yet, but if yall have any opinions on the API/rewrite, please tell me :D

WalshyDev commented 8 months ago

Thank you for the ADR <3

Will do a initial review this weekend

JustAHuman-xD commented 8 months ago

Do you plan on/is it possible to add the serializing to file to be editable component to this rewrite? It was a big part of previously proposed ideas for the rewrite, allowing for the recipes to be edited via editing the file not unlike recipes in datapacks and mods.

JustAHuman-xD commented 8 months ago

This is honestly more so a question to Walshy / Other devs, in some parts of the rewrite you have labeled most local scope variables final, I do the same thing myself but while it isn’t specifically agains the code style guidelines provided for Slimefun it doesn’t follow the code style of the rest of the code. So my question to other devs: Should we remove all of that or is it okay? The code style guidelines are already in dire need of updating as has been discussed in #developers so I’m not entirely sure.

WalshyDev commented 8 months ago

in some parts of the rewrite you have labeled most local scope variables final, I do the same thing myself but while it isn’t specifically agains the code style guidelines provided for Slimefun it doesn’t follow the code style of the rest of the code. So my question to other devs: Should we remove all of that or is it okay? The code style guidelines are already in dire need of updating as has been discussed in #developers so I’m not entirely sure.

so, this is something I do as well. It's imo good practice to always mark things as final (just like it is in JS and others) It can also help with optimisation but is very much a micro-optimisation.

However, this is considered against the style guide today and PRs have had to do changes due to usage in the past.

So, today yes. However, as you said, we really should update the guide and this is something we can bring up as an open question.


Also, not forgot about this pr. It's top of my list, I've just been sick for the past week. I'll get to it when I can :)

SchnTgaiSpock commented 8 months ago

Do you plan on/is it possible to add the serializing to file to be editable component to this rewrite? It was a big part of previously proposed ideas for the rewrite, allowing for the recipes to be edited via editing the file not unlike recipes in datapacks and mods.

i think its possible, ill give it a shot

WalshyDev commented 8 months ago

This PR is massive, I assume due to migrating everything to the new API. We shouldn't do the migration here, not only does it make changing the API a pain it just makes this PR unreviewable.

Can we pull out all the migration stuff to another branch and keep this strictly to the new API?

GitHub barely functions in 240 file PRs (along with this just generally not being a great idea to do such a massive change at once).

J3fftw1 commented 8 months ago

Now that we are on the latest of MockBukkit i would like all the new behaviour and also the old behaviour to be unit tested. So we just make sure everything works. This is a huge change and there is no way we wont forget something unit tests should help with that.

SchnTgaiSpock commented 8 months ago

kk

SchnTgaiSpock commented 8 months ago

man i shouldve done commits more often

i will close this branch and open a new pr just with the new api since that would probably take less time than checking all the migration commits

SchnTgaiSpock commented 8 months ago

the new API only pr is here #4093