GH-959 Move warp contents from language files.#1172
Conversation
![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 successfully refactors the warp inventory configuration from language files into a dedicated warp-inventory.yml, which is a significant improvement for maintainability. The adoption of a repository pattern with asynchronous, transactional operations is a solid architectural choice. However, I've identified a few critical issues, including a race condition and a bug where changes are not persisted, which could lead to data inconsistency. My review provides detailed feedback and suggestions to address these problems by better utilizing the new repository pattern.
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/inventory/WarpInventory.java
Outdated
Show resolved
Hide resolved
...re/src/main/java/com/eternalcode/core/feature/warp/inventory/WarpInventoryConfigService.java
Show resolved
Hide resolved
...ode/core/configuration/migrations/Migration_0010_Move_WarpInventory_to_dedicated_config.java
Outdated
Show resolved
Hide resolved
...core-core/src/main/java/com/eternalcode/core/feature/warp/inventory/WarpInventoryConfig.java
Outdated
Show resolved
Hide resolved
...e/src/main/java/com/eternalcode/core/feature/warp/inventory/WarpInventoryRepositoryImpl.java
Outdated
Show resolved
Hide resolved
...core-core/src/main/java/com/eternalcode/core/feature/warp/inventory/WarpInventoryConfig.java
Outdated
Show resolved
Hide resolved
...core-core/src/main/java/com/eternalcode/core/feature/warp/inventory/WarpInventoryConfig.java
Outdated
Show resolved
Hide resolved
...e/src/main/java/com/eternalcode/core/feature/warp/inventory/WarpInventoryRepositoryImpl.java
Outdated
Show resolved
Hide resolved
CitralFlo
left a comment
CitralFlo
left a comment
There was a problem hiding this comment.
I agree with Rollczi that the repository class it's complicated and could cause problems when maintaining the code in the future
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/inventory/WarpInventory.java
Outdated
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/inventory/WarpInventory.java
Outdated
Show resolved
Hide resolved
Jakubk15
left a comment
Jakubk15
left a comment
There was a problem hiding this comment.
Please test the changes and ensure the behavior of the inventories is proper
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/inventory/WarpInventory.java
Outdated
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/inventory/WarpInventory.java
Outdated
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/inventory/WarpInventory.java
Outdated
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/inventory/WarpInventory.java
Outdated
Show resolved
Hide resolved
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/inventory/WarpInventory.java
Show resolved
Hide resolved
Rollczi
left a comment
Rollczi
left a comment
There was a problem hiding this comment.
dobra jednak nie, te repo z completable feature trochę mnie trigeruje słabo ten kod wygląda dużo nam to komplikuje, nie możemy tego ładniej zrobić? np. removeWarp to już tam się dzieją dziwne rzeczy
…ync handling and configuration managementRemove `WarpInventoryRepository` and migrate its responsibilities to `WarpInventoryConfigService`.
# Conflicts: # eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/messages/ENWarpMessages.java # eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/messages/PLWarpMessages.java
…, maintainability, and structure
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a significant refactoring that moves the warp inventory configuration out of language files into its own dedicated configuration file, warp-inventory.yml. This is a great improvement for separating configuration from translation. The changes include adding a new migration script, creating new configuration and service classes for the warp inventory, converting WarpImpl to a Java Record, and refactoring WarpTeleportService which also includes an important bug fix. Overall, the changes are well-structured and improve the maintainability of the warp feature. I've identified a high-severity issue where changes to warp item slots are not persisted, and a medium-severity issue regarding error handling in the new migration script.
eternalcore-core/src/main/java/com/eternalcode/core/feature/warp/inventory/WarpInventory.java
Show resolved
Hide resolved
...code/core/configuration/migrations/Migration_0035_Move_warp_inventory_to_dedicated_file.java
Outdated
Show resolved
Hide resolved
…improved async handling, and better configuration management; introduce Warp interface and enhance migration logging.
7 participants
Fixes: GH-959