GH-152 Improve dependency injector system #152
Conversation
imDMK
changed the title
Summary of ChangesHello @imDMK, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request represents a significant internal architectural refactoring, replacing the plugin's custom module and binding system with a more modern, annotation-driven component management framework. This change aims to streamline the plugin's startup process, enhance maintainability, and provide a more flexible and extensible foundation for future development by automating the discovery and lifecycle management of various plugin components like services, configurations, and event handlers. The core plugin class is now much leaner, reflecting the delegation of responsibilities to the new component system. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant architectural improvement by replacing the custom module-based dependency injection with a more flexible and powerful annotation-driven component model. This change simplifies service registration and management. My review highlights a couple of critical issues with the new implementation: a compilation error in the component setup and the absence of shutdown logic in the plugin's disable phase, which could lead to resource leaks. I have also pointed out a minor inconsistency in a validation message. Overall, this is a very positive refactoring that will enhance the codebase's maintainability once these points are addressed.
| metrics = new Metrics(plugin, PLUGIN_METRICS_ID); | ||
| ComponentManager componentManager = new ComponentManager(injector, this.getClass().getPackageName()) | ||
| .setPriorityProvider(new AnnotationPriorityProvider()) | ||
| .addProcessor(); |
There was a problem hiding this comment.
This call to .addProcessor() is incomplete and will cause a compilation error because the method requires an argument. It appears you intended to register processors for the new component annotations (like @Service, @Listener, etc.). You'll need to add the concrete processor implementations for each component type. To fix the compilation issue for now, I suggest commenting out this line and adding a TODO as a reminder.
// .addProcessor(); // TODO: Add component processors| void disable() { | ||
| Validator.ifNotNull(configManager, (manager) -> { | ||
| manager.saveAll(); | ||
| manager.clearAll(); | ||
| }); | ||
| Validator.ifNotNull(repositoryManager, RepositoryManager::close); | ||
| Validator.ifNotNull(databaseManager, DatabaseManager::shutdown); | ||
| Validator.ifNotNull(messageService, MessageService::shutdown); | ||
| Validator.ifNotNull(taskScheduler, TaskScheduler::shutdown); | ||
| Validator.ifNotNull(liteCommands, LiteCommands::unregister); | ||
| Validator.ifNotNull(metrics, Metrics::shutdown); | ||
|
|
||
| PlayTimeApiProvider.unregister(); | ||
|
|
||
| logger.info("%s plugin disabled successfully.", PREFIX); | ||
| } |
There was a problem hiding this comment.
The disable() method is empty, which is a major regression from the previous implementation. The old version correctly shut down various services (saving configs, closing database connections, stopping schedulers) to prevent resource leaks and ensure data is saved. This critical shutdown logic needs to be restored within the new component-based architecture. A common pattern is to have components that require cleanup implement an interface like AutoCloseable, and then have the ComponentManager trigger their shutdown during the disable phase.
void disable() {
// TODO: Implement shutdown logic for all managed components, for example:
// componentManager.shutdownAll();
}| ) { | ||
| this.logger = Validator.notNull(logger, "logger"); | ||
| this.configManager = Validator.notNull(configManager, "configManager"); | ||
| this.configService = Validator.notNull(configService, "configManager"); |
There was a problem hiding this comment.
The validation message for configService uses the string "configManager". This is inconsistent and could be misleading if a NullPointerException occurs, as the error message would refer to configManager. For clarity and easier debugging, the name in the error message should match the variable being checked.
| this.configService = Validator.notNull(configService, "configManager"); | |
| this.configService = Validator.notNull(configService, "configService"); |
No description provided.