Dependency injection#79
Conversation
|
@Muspah, I would appreciate a second opinion on this; sorry for the massive diff. |
|
PS: hiding whitespace changes helps ;) |
Muspah
left a comment
There was a problem hiding this comment.
LGTM, it's "boring" stuff but it needs to be done to overhaul all the legacy crap that's in the codebase. Thanks very much for your continuous effort.
Are we able to run some real-world tests before releasing to Spigot/Paper/...? I do run a small demo server, but without many players except for myself.
Thanks! I did some quick testing on my test setup - mostly to make sure commands, challenges, etc. still work as expected. I can update to our live server and see if we run into issues. We don't have a ton of players in skyblock, but a handful of regulars. |
|
Sounds good. Feel free to merge when you're ready and I will include it into my SQL-support branch when possible so it gets tested on my demo too. Lets just prevent that it gets included in some trivial MC version update release without testing :) |
|
Sounds good! Can we create a release for spigot & paper at the current master head (4d7afec)? It includes a bug fix for 1.21.4, biome updates, and a couple other things that should be stable (we are running it in production). It is compatible with [1.20.5, 1.21.4]. |
- Make most dependencies obvious by moving injections to the constructor - Break some cyclic dependencies - Reduce reliance on the main plugin class
2ec9fad to
3485366
Compare
The goal of this PR is to improve the maintainability of the plugin by moving towards dependency-injection-based initialization.
Current issue:
The plugin currently uses a combination of static references, a massive single-instance service locator (the main uSkyBlock class), and passing dependencies down to child objects. This means
This PR breaks these dependencies through dependency injection using Guice.
Initial testing looks good, but since this is a larger change, it may cause some initial instability or hidden issues.
Closes #14
Resolves #76