Access Changed from public to protected for TarantoolResultImpl and TarantoolTupleResultImpl #326#338
Conversation
akudiyar
left a comment
There was a problem hiding this comment.
Hi, thanks for the patch! Please look at the comments below.
...main/java/io/tarantool/driver/core/metadata/RowsMetadataToTarantoolTupleResultConverter.java
Outdated
Show resolved
Hide resolved
.../java/io/tarantool/driver/mappers/converters/value/ArrayValueToTarantoolResultConverter.java
Outdated
Show resolved
Hide resolved
.../io/tarantool/driver/mappers/converters/value/ArrayValueToTarantoolTupleResultConverter.java
Outdated
Show resolved
Hide resolved
|
@akudiyar I have modified and tried to implement as per suggestions, Please look into the new changes and let me know if any further changes are required. |
akudiyar
left a comment
There was a problem hiding this comment.
Hi @devrishal, would you mind rebasing your PR?
Also, I suggest using TarantolResultImplFactory as a singleton. And try to run mvn verify and mvn test before pushing, to eliminate any missing test changes and linter warnings.
...main/java/io/tarantool/driver/core/metadata/RowsMetadataToTarantoolTupleResultConverter.java
Outdated
Show resolved
Hide resolved
akudiyar
left a comment
There was a problem hiding this comment.
Two more places to introduce the singleton usage + one small fix with typing and the Javadocs are missing for the new class.
Please run also mvn clean javadoc:javadoc to fix all the new linter warnings.
.../java/io/tarantool/driver/mappers/converters/value/ArrayValueToTarantoolResultConverter.java
Outdated
Show resolved
Hide resolved
src/main/java/io/tarantool/driver/core/TarantoolResultFactory.java
Outdated
Show resolved
Hide resolved
akudiyar
left a comment
There was a problem hiding this comment.
Looks good now. Thanks for your work Rishal!
|
Although it shows me some conflicts, so I cannot merge the PR. Would you mind rebasing it again? @devrishal |
Did the rebase and Thanks for providing your guidance and suggestions. |
|
@devrishal you did a "merge" but what is needed is rebasing your branch on the master branch. We have a policy that enforces only fast-forward pushes. Would you mind removing the last merge commit and rebasing again? |
@akudiyar I have tried the approach, Please let me know if the things looks good now? |
e8aa289 to
2825046
Compare
…ultImpl and TarantoolTupleResultImpl
Removed Type variable from class level.
|
@akudiyar Can you please check and let me know if there is any further changes needed in my PR |
For
TarantoolResultImpl,TarantoolTupleResultImplChanging the access to protected because of its internal functionality. They are used in converters.
I haven't forgotten about:
Related issues:
Closes #326