-
Notifications
You must be signed in to change notification settings - Fork 6
Add a new components to CDI. #298
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Thanks. LGTM |
c23d2c0
to
c7decfd
Compare
c7decfd
to
884c08e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I made a minor suggestion.
import static org.junit.jupiter.api.Assertions.assertEquals; | ||
import static org.junit.jupiter.api.Assertions.assertNotNull; | ||
import static org.junit.jupiter.api.Assertions.assertTrue; | ||
import static org.junit.jupiter.api.Assertions.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please do not use *
.
b704291
to
3f5da36
Compare
# Conflicts: # deployment/src/main/java/io/quarkiverse/doma/deployment/DomaBuildTimeConfig.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a few minor comments.
I'll approve it
this.duplicateColumnHandler = duplicateColumnHandler; | ||
this.sqlBuilderSettings = sqlBuilderSettings; | ||
this.statisticManager = statisticManager; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to have null checks for the following parameters, just like the other parameters.
- duplicateColumnHandler
- sqlBuilderSettings
- statisticManager
@@ -115,4 +126,25 @@ public interface DataSourceBuildTimeConfig { | |||
@ConfigDocDefault("import.sql in DEV, TEST ; no-file otherwise") | |||
Optional<String> sqlLoadScript(); | |||
} | |||
|
|||
@ConfigGroup | |||
public interface SqlBuilderBuildTimeConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SqlBuilderSettingsBuildTimeConfig might be a more consistent name, though it is somewhat redundant.
@Singleton | ||
@DefaultBean | ||
DuplicateColumnHandler duplicateColumnHandler() { | ||
return duplicateColumnHandler; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems better to have a null check to align with other parts.
In the Pull Request description, should sql-builder-settings.should-remove-blank-lines actually be quarkus.doma.sql-builder-settings.should-remove-blank-lines? |
The following components added to Doma2 will be managed by CDI.
Add new configuration property:
If DuplicateColumnHandler or SqlBuilderSettings is registered in CDI, these properties will be ignored.