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

✨ feat(api): Introduce SlimefunItemRegistryFinalizedEvent #4099

Merged
merged 10 commits into from
Jan 18, 2024
Merged

✨ feat(api): Introduce SlimefunItemRegistryFinalizedEvent #4099

merged 10 commits into from
Jan 18, 2024

Conversation

ProfElements
Copy link
Contributor

@ProfElements ProfElements commented Jan 17, 2024

Description

This Event is provided so that plugins that have integrations with each other aren't stuck in a state in which some SlimefunItems may not be available to one plugin or the other.

I will use this event to provide a cleaner integration between Dynatech and other plugins. Currently InfinityXpansion, ExoticGarden, and Gastronomicon.

Proposed changes

Make an event to be able to listen to Slimefun's Registry finalizing all static items.

Checklist

  • I have fully tested the proposed changes and promise that they will not break everything into chaos.
  • I have also tested the proposed changes in combination with various popular addons and can confirm my changes do not break them.
  • I have made sure that the proposed changes do not break compatibility across the supported Minecraft versions (1.16.* - 1.20.*).
  • I followed the existing code standards and didn't mess up the formatting.
  • I did my best to add documentation to any public classes or methods I added.
  • I have added Nonnull and Nullable annotations to my methods to indicate their behaviour for null values
  • I added sufficient Unit Tests to cover my code.

This Event is provided so that plugins that have integrations with each other aren't
stuck in a state in which some SlimefunItems may not be available to one plugin or the other.

I will use this event to provide a cleaner integration between Dynatech and other plugins.
Currently InfinityXpansion, ExoticGarden, and Gastronomicon.
@ProfElements ProfElements requested a review from a team as a code owner January 17, 2024 09:02
Copy link
Contributor

Pro Tip!
You can help us label your Pull Requests by using the following branch naming convention next time you create a pull request. ❤️

Branch naming convention Label
feature/** 🎈 Feature
fix/** ✨ Fix
chore/** 🧹 Chores
api/** 🔧 API
performance/** 💡 Performance Optimization
compatibility/** 🤝 Compatibility

If your changes do not fall into any of these categories, don't worry. You can just ignore this message in that case! 👀

@J3fftw1
Copy link
Contributor

J3fftw1 commented Jan 17, 2024

Can we get unit tests?

Copy link
Contributor

github-actions bot commented Jan 17, 2024

Slimefun preview build

A Slimefun preview build is available for testing!
Commit: a25a4e43

https://preview-builds.walshy.dev/download/Slimefun/4099/a25a4e43

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!

…FinalizedEvent.

Moved call to event upward to make it more readable.
Remove `before auto loading` as it was inrelevant to code after a second read.
@ProfElements
Copy link
Contributor Author

Fixed up according to suggestions

@JustAHuman-xD
Copy link
Contributor

I'm not sure about the use case of this tbch, for example: if one addon waits for this event to then register its items to integrate with another addon, but then another addon waits for this event to integrate with that addon, then you would still have the same problem.

Currently if you soft depend on any addon you can utilize its items without any problem, in my experience anyways. I guess it depends on the addon but I've never experienced that not working.

The use case you have also provided encourages registering items, after load items is called, which feels wrong.

Idk this could just be me but I don't think this is the right solution.

@ProfElements
Copy link
Contributor Author

ProfElements commented Jan 17, 2024

I'm not sure about the use case of this tbch, for example: if one addon waits for this event to then register its items to integrate with another addon, but then another addon waits for this event to integrate with that addon, then you would still have the same problem.

Currently if you soft depend on any addon you can utilize its items without any problem, in my experience anyways. I guess it depends on the addon but I've never experienced that not working.

The use case you have also provided encourages registering items, after load items is called, which feels wrong.

Idk this could just be me but I don't think this is the right solution.

This probably isn't the right solution, but I'm currently in the situation when I want to integrate items into another project. Which I can't load after because its softdepend on DynaTech. I don't thing we should add items to registry after this point, I'm using it to add all my recipes using another projects items IE my SeedPlucker adding Gastro Seeds to its recipes. If you have a circular softdepend ie DynaTech softdepend on Gastronomicon, and Gastronomicon softdepend on DynaTech, they are loaded arbitrarily.

Co-authored-by: JustAHuman-xD <[email protected]>
@JustAHuman-xD
Copy link
Contributor

This probably isn't the right solution, but I'm currently in the situation when I want to integrate items into another project. Which I can't load after because its softdepend on DynaTech. I don't thing we should add items to registry after this point, I'm using it to add all my recipes using another projects items IE my SeedPlucker adding Gastro Seeds to its recipes

Ah so you're in a circular dependency problem. Which is similar to the issue I showed with this. One solution I can think of is you PR to gastro and make gastro add it to the seed plucker but yeah I get the problem.

I think what you may be able to do is something kinda like slimefun, slimefun adds a lot of recipes to machines on post register. So if all you want to do is add machine recipes, then this event is actually a good solution. In which case I misunderstood what this was meant for. I thought you meant registering items dependent on addons items 😅

@ProfElements
Copy link
Contributor Author

Yeah Im sorry i know my explanation was kinda confusing

Co-authored-by: JustAHuman-xD <[email protected]>
JustAHuman-xD
JustAHuman-xD previously approved these changes Jan 17, 2024
Copy link
Contributor

@JustAHuman-xD JustAHuman-xD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, apologizes for the confusion 😅

Copy link
Contributor

@JustAHuman-xD JustAHuman-xD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(You double removed the empty line here, not a big deal I'll re add it lol)

JustAHuman-xD
JustAHuman-xD previously approved these changes Jan 17, 2024
Copy link
Contributor

@JustAHuman-xD JustAHuman-xD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@J3fftw1 J3fftw1 changed the title ✨ feat(api): Introduce SlimefunRegistryFinalizedEvent ✨ feat(api): Introduce SlimefunItemRegistryFinalizedEvent Jan 17, 2024
J3fftw1
J3fftw1 previously approved these changes Jan 17, 2024
@WalshyDev WalshyDev dismissed stale reviews from J3fftw1 and JustAHuman-xD via a3c74f8 January 17, 2024 16:32
@WalshyDev WalshyDev merged commit 7c917c3 into Slimefun:master Jan 18, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants