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
Feature/adding some generics #5830
base: master
Are you sure you want to change the base?
Feature/adding some generics #5830
Conversation
make the code more generic
This reverts commit 60bc759.
Signed-off-by: MichaelKern-IVV <102645261+MichaelKern-IVV@users.noreply.github.com>
liquibase-standard/src/main/java/liquibase/executor/jvm/JdbcExecutor.java
Fixed
Show resolved
Hide resolved
liquibase-standard/src/main/java/liquibase/executor/jvm/SingleColumnRowMapper.java
Fixed
Show resolved
Hide resolved
liquibase-standard/src/main/java/liquibase/diff/Difference.java
Dismissed
Show resolved
Hide resolved
@@ -12,7 +12,7 @@ | |||
/** | |||
* @deprecated Implement commands with {@link liquibase.command.CommandStep} and call them with {@link liquibase.command.CommandFactory#getCommandDefinition(String...)}. | |||
*/ | |||
public class HistoryCommand extends AbstractCommand { | |||
public class HistoryCommand extends AbstractCommand<CommandResult> { |
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.
Not sure if this could have a negative impact, but we try to avoid updating public elements (return types, method signatures, class definition, etc) as in some cases it could be a breaking change for some people depending on how they are using methods/classes. Maybe this case doesn't hurt, but considering our guidelines I should revert this one.
@@ -31,7 +31,7 @@ public ColumnMapRowMapper(boolean caseSensitiveDatabase) { | |||
} | |||
|
|||
@Override | |||
public Object mapRow(ResultSet rs, int rowNum) throws SQLException { | |||
public Map<String,?> mapRow(ResultSet rs, int rowNum) throws SQLException { |
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.
Same here.
@Override | ||
public Object doInStatement(Statement stmt) throws SQLException, DatabaseException { | ||
public Integer doInStatement(Statement stmt) throws SQLException, DatabaseException { |
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 this case I would leave it as it was because it's updating a public method and there is only one place where it is cast to an Integer.
Hi @MichaelKern-IVV, Thank you for creating this enhancement PR, as mentioned in some comments we try to avoid updating Thanks, |
@@ -36,11 +36,11 @@ public RowMapperNotNullConstraintsResultSetExtractor(RowMapper rowMapper) { | |||
} | |||
|
|||
@Override | |||
public Object extractData(ResultSet resultSet) throws SQLException { | |||
List<Object> resultList = (this.rowsExpected > 0 ? new ArrayList<>(this.rowsExpected) : new ArrayList<>()); | |||
public List<Map<String,?>> extractData(ResultSet resultSet) throws SQLException { |
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.
Same here
@@ -5,12 +5,10 @@ | |||
public class CachedRow { | |||
private final Map row; | |||
|
|||
public CachedRow(Map row) { | |||
public CachedRow(Map<String,?> row) { |
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.
Same here
@@ -16,8 +16,8 @@ public abstract class NumberUtil { | |||
* @deprecated use {@link ObjectUtil#convert(Object, Class)} | |||
*/ | |||
@Deprecated | |||
public static Number convertNumberToTargetClass(Number number, Class targetClass) throws IllegalArgumentException { | |||
return (Number) ObjectUtil.convert(number, targetClass); | |||
public static <T extends Number> T convertNumberToTargetClass(Number number, Class<T> targetClass) throws IllegalArgumentException { |
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 as this is a deprecated method it's not worth to update it.
this.sql = sql; | ||
this.rse = rse; | ||
} | ||
|
||
|
||
@Override | ||
public Object doInCallableStatement(CallableStatement cs) throws SQLException, DatabaseException { | ||
public List<T> doInCallableStatement(CallableStatement cs) throws SQLException, DatabaseException { |
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.
Same here
Impact
Description
Make more use of generics in order to