[WIP] Add dependency injection functionality, Closes #85#86
[WIP] Add dependency injection functionality, Closes #85#86aikar merged 6 commits intoaikar:masterfrom
Conversation
|
possible improvements:
|
aikar
left a comment
aikar
left a comment
There was a problem hiding this comment.
Please remove all .toArray with 0 changes. It's more optimized to pass the intended size, or else the toArray call is going to throw away the array you passed and recreate it anyways. Lets not undo already done optimizations.
|
|
||
| throw new InvalidCommandArgument(false); | ||
| } | ||
| return players.toArray(new OnlinePlayer[players.size()]); |
| } | ||
|
|
||
| String[] result = args.toArray(new String[args.size()]); | ||
| String[] result = args.toArray(new String[0]); |
| * @throws IllegalArgumentException when instance isn't an instance of clazz | ||
| * @throws IllegalStateException when there is already an instance for the provided class registered | ||
| */ | ||
| public void registerDependency(Class<?> clazz, Object instance){ |
There was a problem hiding this comment.
Use <T> with Class <T> clazz, T instance, to require it actually satisfies the class its providing for.
There was a problem hiding this comment.
public <T> void registerDependency(Class<T> clazz, T instance) {There was a problem hiding this comment.
I had this and it didn't work. when I want to pass the plugins class, I can't cast to it, thus throwing a compiler error. https://i.imgur.com/ynHskze.png
| * @throws IllegalArgumentException when instance isn't an instance of clazz | ||
| * @throws IllegalStateException when there is already an instance for the provided class registered | ||
| */ | ||
| public void registerNamedDependency(Class<?> clazz, String key, Object instance){ |
There was a problem hiding this comment.
Same as above for <T>, but just make this registerDependency, since its a different signature
| * @throws IllegalStateException when there is already an instance for the provided class registered | ||
| */ | ||
| public void registerNamedDependency(Class<?> clazz, String key, Object instance){ | ||
| if(!clazz.isInstance(instance)){ |
There was a problem hiding this comment.
Not necessary with <T> restrictions
| throw new IllegalArgumentException(instance.getClass().getName() + " isn't a subclass of " + clazz.getName() + "!"); | ||
| } | ||
| if(dependencies.containsRow(clazz) && dependencies.containsColumn(key)){ | ||
| throw new IllegalStateException("There is already an instance of " + clazz.getName() + " with the key " + key + " registered!"); |
There was a problem hiding this comment.
Not sure we should throw, but just let it overwrite, maybe return old, like a typical put operation.
| * | ||
| * @param baseCommand the instance which fields should be filled | ||
| */ | ||
| public void inject(BaseCommand baseCommand){ |
There was a problem hiding this comment.
I think also more descriptive "injectDependencies"
| Map<String, Object> resolveContexts(CommandIssuer sender, List<String> args, int argLimit) throws InvalidCommandArgument { | ||
| args = Lists.newArrayList(args); | ||
| String[] origArgs = args.toArray(new String[args.size()]); | ||
| String[] origArgs = args.toArray(new String[0]); |
| throw new InvalidCommandArgument(false); | ||
| } | ||
| return players.toArray(new OnlinePlayer[players.size()]); | ||
| return players.toArray(new OnlinePlayer[0]); |
|
if some bungee or sponge ppl could suggest default dependencies or the platform that would be great. |
aikar
left a comment
aikar
left a comment
There was a problem hiding this comment.
just few more changes then looks good to me.
| try { | ||
| field.setAccessible(true); | ||
| field.set(baseCommand, object); | ||
| field.setAccessible(false); |
There was a problem hiding this comment.
Don't know if theres a reason to set this to false? what if it was already accessible before.
| * @param baseCommand the instance which fields should be filled | ||
| */ | ||
| void injectDependencies(BaseCommand baseCommand){ | ||
| for (Field field : baseCommand.getClass().getDeclaredFields()) { |
There was a problem hiding this comment.
If getDeclaredFields is the one that only includes its own fields, then switch to the other. we want to include parent class fields too, but I'm not at a java doc atm.
There was a problem hiding this comment.
when we don't use getDeclaredFields, we can't inject private fields. do we want to just iterate over both?
|
|
||
| //TODO more default dependencies for sponge | ||
| registerDependency(plugin.getClass(), plugin); | ||
| registerDependency(PluginContainer.class, plugin); |
There was a problem hiding this comment.
Remove the PluginContainer one since per @kashike its not the same thing.
aikar
changed the title
2 participants
also ran some intellij inspections.