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

Add Dependency Injection Support for WorldGen Plugins #5212

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

spookynutz
Copy link

Hello,

I'm opening this PR to work on the implementation of dependency injection for the WorldGen Plugins.
This PR is opened as a draft as a base for discussion since I'm new to the code and don't quite understand how things work yet.

Contains

Implementation for proposal in #5003

@spookynutz
Copy link
Author

My main interrogation was where this manager should be used. My assumption from what I found was to replace this here :

ModuleEnvironment environment = context.get(ModuleManager.class).getEnvironment();
context.put(WorldGeneratorPluginLibrary.class, new DefaultWorldGeneratorPluginLibrary(environment, context));

Could anyone confirm this ?

@BenjaminAmos
Copy link
Contributor

Although, I am wondering if it would be easier to implement this by just calling InjectionManager.inject(item) in DefaultWorldGeneratorPluginLibrary#instantiateAllOfType:

@Override
public <U extends WorldGeneratorPlugin> List<U> instantiateAllOfType(Class<U> ofType) {
List<U> result = Lists.newArrayList();
for (ClassMetadata<?, ?> classMetadata : library) {
if (ofType.isAssignableFrom(classMetadata.getType())
&& classMetadata.isConstructable()
&& classMetadata.getType().getAnnotation(RegisterPlugin.class) != null) {
U item = ofType.cast(classMetadata.newInstance());
if (item != null) {
result.add(item);
}
}
}
return result;
}

@jdrueckert
Copy link
Member

@spookynutz Did the review comments help or are you still blocked somehow? Are you planning to continue with this?

@jdrueckert jdrueckert added the Status: Needs Author Input Requires more information by the author on the reported issue or provided changes label Apr 21, 2024
@spookynutz
Copy link
Author

Although, I am wondering if it would be easier to implement this by just calling InjectionManager.inject(item) in DefaultWorldGeneratorPluginLibrary#instantiateAllOfType:

@Override
public <U extends WorldGeneratorPlugin> List<U> instantiateAllOfType(Class<U> ofType) {
List<U> result = Lists.newArrayList();
for (ClassMetadata<?, ?> classMetadata : library) {
if (ofType.isAssignableFrom(classMetadata.getType())
&& classMetadata.isConstructable()
&& classMetadata.getType().getAnnotation(RegisterPlugin.class) != null) {
U item = ofType.cast(classMetadata.newInstance());
if (item != null) {
result.add(item);
}
}
}
return result;
}

Hello @BenjaminAmos, @jdrueckert, sorry for the delayed response I got taken by work and did not have time to advance the issue ...
I'm not sure I understand correctly this comment. Are you suggesting to drop the creation of WorldGeneratorPluginManager and to rather use

public static <T> T inject(final T object, Context context) {
around ?

@BenjaminAmos
Copy link
Contributor

Are you suggesting to drop the creation of WorldGeneratorPluginManager and to rather use

public static <T> T inject(final T object, Context context) {

around

Yes, that is essentially what I think I meant (it was a while ago now). Although the current code is rather guilty of this already, I would prefer that new code does not introduce unecessary abstractions (the less code the better, prioritising readability).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Needs Author Input Requires more information by the author on the reported issue or provided changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants