Use Dependency Injection for service initialization #707
Conversation
| public @NotNull WorldHandler getWorldHandler(){ | ||
| return application.container().getService(WorldHandler.class); | ||
| } | ||
|
|
||
| public SmoothTeleport getSmoothTeleport() { | ||
| return smoothTeleport; | ||
| public @NotNull SmoothTeleport getSmoothTeleport() { | ||
| return application.container().getService(SmoothTeleport.class); | ||
| } | ||
|
|
||
| public AsyncManager getAsyncManager() { | ||
| return asyncManager; | ||
| public @NotNull AsyncManager getAsyncManager() { | ||
| return application.container().getService(AsyncManager.class); | ||
| } | ||
|
|
||
| public @NotNull WreckManager getWreckManager(){ | ||
| return wreckManager; | ||
| return application.container().getService(WreckManager.class); |
There was a problem hiding this comment.
Should we deprecate these and instead expose getService?
There was a problem hiding this comment.
Nah but it might be good to expose an API class that holds this stuff or something - exposing the DI container is an anti-pattern typically
| boolean isAsync(); | ||
|
|
||
| int getPeriod(); |
There was a problem hiding this comment.
Should we default these to false and 1 second?
There was a problem hiding this comment.
Looking at the usages internally, they mostly are 1 tick services. However, for users of the API I think it makes more sense to maintain a 1 second default so as to not provide a possible foot-gun.
There was a problem hiding this comment.
This isn't really intended for external users/addons - if we do see value here I would probably move this stuff to its own package and build it out a bit more.
There was a problem hiding this comment.
There's a lot of items in the addons that could utilize this type of thing. I'd like to see us build it in such a way it can be used internally and externally.
|
|
||
| import org.jetbrains.annotations.NotNull; | ||
|
|
||
| public record VersionInfo(String version) { |
There was a problem hiding this comment.
Should we call this MinecraftVersionInfo to reduce confusion with Movecraft versioning?
|
|
||
| import java.util.logging.Logger; | ||
|
|
||
| public class VersionProvider implements Provider<VersionInfo> { |
| @Deprecated | ||
| public static @NotNull String getPackageName(@NotNull String minecraftVersion) { | ||
| String[] parts = minecraftVersion.split("\\."); | ||
| if (parts.length < 2) |
There was a problem hiding this comment.
If this is no longer used we should remove it.
There was a problem hiding this comment.
its still used for now
There was a problem hiding this comment.
Can this be merged into Settings as one class? I think it'd be far easier to maintain if they are together.
There was a problem hiding this comment.
the intent is to change Settings to eventually be non-static. seperating them helps for this purpose
Describe in detail what your pull request accomplishes
Servicefor startup/stop patternsWorkerfor an async/sync background processorsVersionInfoto access version related dataServiceSettingsto be injected, but that is out of scope for nowServicesingletonsBukkitRunnableimplementations toWorkerChecklist