[kbl2vec] Logger aware ConverterRegistry#377
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request makes the ConverterRegistry logger-aware by adding a Logger parameter to its constructor. The change enables better logging control and dependency injection by accepting a logger instance instead of using a hardcoded static logger reference.
- Added Logger parameter to ConverterRegistry constructor
- Updated TransformationContextImpl to accept and use injected Logger
- Modified all instantiations to pass the transform logger explicitly
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| ConverterRegistry.java | Added Logger parameter to constructor and stored as field |
| TransformationContextImpl.java | Modified constructor to accept Logger parameter instead of using static reference |
| ConversionOrchestrator.java | Updated to pass transform logger to dependencies |
| TestConversionOrchestrator.java | Updated test setup to pass transform logger |
| DeterministicXmlIdGeneratorTest.java | Updated test setup to pass transform logger and simplified imports |
| private final Logger transformLogger; | ||
|
|
||
| public ConverterRegistry(final ConversionProperties conversionProperties) { | ||
| public ConverterRegistry(final Logger transformLogger, final ConversionProperties conversionProperties) { | ||
| this.transformLogger = transformLogger; |
There was a problem hiding this comment.
The Logger field is added but never used within the ConverterRegistry class. If logging functionality is intended, consider adding appropriate log statements or removing the unused field to avoid confusion.
|
| private final StringToColorConverter stringToColorConverter; | ||
| private final StringToWireTypeConverter stringToWireTypeConverter; | ||
| private final StringToMaterialConverter stringToMaterialConverter; | ||
| private final Logger transformLogger; |
There was a problem hiding this comment.
I don't quite understand why the ConverterRegistry needs a logger. Will there be dedicated logging for the converter in the future?
There was a problem hiding this comment.
See my comments in the Geometry PR. I should have read the PR completely first, but I was acting to fast. Having seen the complete solution, I think this PR is unnecessary (at least for the time being). I will leave it here, maybe there is a need for logging by the converters in the future.
2 participants
Pull Request
Changes