garydgregory commented on code in PR #355:
URL: https://github.com/apache/commons-dbcp/pull/355#discussion_r1511983504
##########
src/main/java/org/apache/commons/dbcp2/DataSourceConnectionFactory.java:
##########
@@ -16,6 +16,8 @@
*/
package org.apache.commons.dbcp2;
+import static java.util.Objects.isNull;
Review Comment:
We only use static imports for JUnit assertions.
##########
src/main/java/org/apache/commons/dbcp2/AbandonedTrace.java:
##########
@@ -169,7 +169,7 @@ private void init(final AbandonedTrace parent) {
*/
protected void removeThisTrace(final Object source) {
if (source instanceof AbandonedTrace) {
- AbandonedTrace.class.cast(source).removeTrace(this);
+ ((AbandonedTrace) source).removeTrace(this);
Review Comment:
Why is this change needed?
##########
src/main/java/org/apache/commons/dbcp2/BasicDataSource.java:
##########
@@ -297,7 +297,7 @@ protected static void validateConnectionFactory(final
PoolableConnectionFactory
private String connectionString;
/**
- * The connection user name to be passed to our JDBC driver to establish a
connection.
+ * The connection username to be passed to our JDBC driver to establish a
connection.
Review Comment:
Let's not use compound nouns when we don't have to.
##########
src/main/java/org/apache/commons/dbcp2/DelegatingDatabaseMetaData.java:
##########
@@ -985,9 +985,6 @@ public ResultSet getVersionColumns(final String catalog,
final String schema, fi
}
protected void handleException(final SQLException e) throws SQLException {
- if (connection == null) {
Review Comment:
This might break subclasses, so don't change it. Unlikely, but possible.
##########
src/main/java/org/apache/commons/dbcp2/BasicDataSource.java:
##########
@@ -1794,7 +1794,7 @@ public void setConnectionFactoryClassName(final String
connectionFactoryClassNam
*/
public void setConnectionInitSqls(final Collection<String>
connectionInitSqls) {
final List<String> collect = Utils.isEmpty(connectionInitSqls) ? null
- : connectionInitSqls.stream().filter(s ->
!isEmpty(s)).collect(Collectors.toList());
+ : connectionInitSqls.stream().filter(s ->
!isEmpty(s)).collect(Collectors.toList());
Review Comment:
Please don't change the formatting, it makes PR noisier and takes longer to
review.
##########
src/main/java/org/apache/commons/dbcp2/BasicDataSource.java:
##########
@@ -1494,7 +1494,7 @@ public Duration getValidationQueryTimeoutDuration() {
* @throws IllegalStateException if invalidating the connection failed.
* @since 2.1
*/
- @SuppressWarnings("resource")
+ @SuppressWarnings({"resource", "RedundantSuppression"})
Review Comment:
Not needed.
##########
src/main/java/org/apache/commons/dbcp2/BasicDataSourceFactory.java:
##########
@@ -440,10 +448,10 @@ private void validatePropertyNames(final Reference ref,
final Name name, final L
// and it is not in the "silent" list, tell user we are ignoring
it.
if (!(ALL_PROPERTY_NAMES.contains(propertyName) ||
NUPROP_WARNTEXT.containsKey(propertyName) ||
SILENT_PROPERTIES.contains(propertyName))) {
final String propertyValue = Objects.toString(ra.getContent(),
null);
- final StringBuilder stringBuilder = new
StringBuilder(nameString);
- stringBuilder.append("Ignoring unknown property:
").append("value of \"").append(propertyValue).append("\" for
\"").append(propertyName)
- .append("\" property");
- infoMessages.add(stringBuilder.toString());
+ final String stringBuilder =
Review Comment:
Why?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
