GH-973 Rename and restructure placeholders configuration classes #1225
Conversation
ChudziudgiToJa
requested review from
CitralFlo,
Jakubk15,
P1otrulla,
Pawelusze,
Qbiterv,
Rollczi,
VerniQ,
igoyek,
imDMK,
mydelkonivea,
noyzys and
vLuckyyy
ChudziudgiToJa
changed the title
![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 placeholder configuration by moving it from a separate file into the main config.yml. This is a good move for consolidation and simplifies management. The introduction of the PlaceholdersSettings interface is a great choice for decoupling the configuration from its usage. The overall implementation is solid. I have one minor suggestion to improve code clarity and maintainability.
| @Override | ||
| public Map<String, String> placeholders() { | ||
| return placeholders; | ||
| } |
There was a problem hiding this comment.
The explicit implementation of the placeholders() method is redundant. The class-level @Getter and @Accessors(fluent = true) annotations will instruct Lombok to generate an identical method that correctly implements the PlaceholdersSettings interface. Removing this manual override will make the code cleaner and less verbose, fully leveraging Lombok's capabilities for boilerplate reduction.
ChudziudgiToJa
requested review from
CitralFlo and
imDMK
and removed request for
CitralFlo,
P1otrulla,
Pawelusze,
Qbiterv,
Rollczi,
VerniQ,
igoyek,
imDMK,
mydelkonivea and
noyzys
|
|
||
| import java.util.Map; | ||
|
|
||
| public interface PlaceholdersSettings { |
There was a problem hiding this comment.
Ogolnie Settings odbieramy jako dane a nie zachowanie (MA SENS) w tym przypadku ten interfejs nie ma sensu dodaje tylko zloznosc bez zadnej wartosci bo nadal zalezy od konceptu Settings. Settings to tak naprawde tzw. Value object czyli javowy rekord/klasa. Interfejs to tylko kontrakt dla zachowan. Sens sie pojawia kiedy masz rozne zrodla konfiguracji wtedy Settings sam w sobie nadal jest value objectem
| class PlaceholdersSetup { | ||
|
|
||
| @Subscribe(EternalInitializeEvent.class) | ||
| void setUp(PlaceholderRegistry placeholders, PlaceholdersConfiguration config) { |
There was a problem hiding this comment.
Musimy tez wiedziec czym jest Configuration vs Config bo raz jest tak raz tak sama w sobie paczka implementation to jest zwykly wyciek abstrakcji niekontrolowany
| @Getter | ||
| @Accessors(fluent = true) |
There was a problem hiding this comment.
| @Getter | |
| @Accessors(fluent = true) |
There was a problem hiding this comment.
No, instead the author should remove the placeholders() getter explicitly. Annotations should be kept unless we are getting rid of the PlaceholdersSettings interface.
|
|
||
| import java.util.Map; | ||
|
|
||
| public interface PlaceholdersSettings { |
There was a problem hiding this comment.
I think an interface isn’t needed here. This is just a placeholders config, and at this point, I’m overengineering it.
| @Getter | ||
| @Accessors(fluent = true) |
There was a problem hiding this comment.
No, instead the author should remove the placeholders() getter explicitly. Annotations should be kept unless we are getting rid of the PlaceholdersSettings interface.
|
Follow other reviews - nothing to add (Commander keeps tagging me) |
No description provided.