Skip to content
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

Minor cleanups of the JDBC code #4441

Merged
merged 2 commits into from
Dec 20, 2018
Merged

Conversation

systay
Copy link
Collaborator

@systay systay commented Dec 11, 2018

Signed-off-by: Andres Taylor [email protected]


This change is Reviewable

Signed-off-by: Andres Taylor <[email protected]>
@systay systay requested a review from sougou as a code owner December 11, 2018 10:31
@systay
Copy link
Collaborator Author

systay commented Dec 11, 2018

@harshit-gangal if you have time to look this over, it would be much appreciated

Copy link
Member

@harshit-gangal harshit-gangal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 12 files at r1.
Reviewable status: 1 of 12 files reviewed, 2 unresolved discussions (waiting on @systay and @sougou)


java/jdbc/src/main/java/io/vitess/jdbc/VitessConnection.java, line 486 at r1 (raw file):

Quoted 13 lines of code…
     * @throws SQLException
     */
    public boolean isClosed() throws SQLException {
        return this.closed;
    }
 
     *
     * @param name - Property Name
     * @return Property Value
     * @throws SQLException
     */
    public String getClientInfo(String name) throws SQLException {
        return null;

This is the signature of the method implemented from Connection interface.
As of now, they are not throwing SQLException. But, any reason for doing this?


java/jdbc/src/test/java/io/vitess/jdbc/ConnectionPropertiesTest.java, line 36 at r1 (raw file):

import static io.vitess.util.Constants.DEFAULT_INCLUDED_FIELDS;
import static io.vitess.util.Constants.DEFAULT_TABLET_TYPE;
import static org.junit.Assert.*;

explicitly import the classes.

Copy link
Collaborator Author

@systay systay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 of 12 files reviewed, 2 unresolved discussions (waiting on @harshit-gangal, @systay, and @sougou)


java/jdbc/src/main/java/io/vitess/jdbc/VitessConnection.java, line 486 at r1 (raw file):

Previously, harshit-gangal wrote…
     * @throws SQLException
     */
    public boolean isClosed() throws SQLException {
        return this.closed;
    }
 
     *
     * @param name - Property Name
     * @return Property Value
     * @throws SQLException
     */
    public String getClientInfo(String name) throws SQLException {
        return null;

This is the signature of the method implemented from Connection interface.
As of now, they are not throwing SQLException. But, any reason for doing this?

When implementing interfaces, it's allowable to throw less than the interface declares, not more. I just feel like it's always is better to minimize the surface exposed when possible. If you want, I can keep the throws declarations as is instead.

Copy link
Member

@harshit-gangal harshit-gangal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 1 of 12 files reviewed, 2 unresolved discussions (waiting on @systay and @sougou)


java/jdbc/src/main/java/io/vitess/jdbc/VitessConnection.java, line 486 at r1 (raw file):

Previously, systay (Andres Taylor) wrote…

When implementing interfaces, it's allowable to throw less than the interface declares, not more. I just feel like it's always is better to minimize the surface exposed when possible. If you want, I can keep the throws declarations as is instead.

I am good with removing exceptions from method signature which are not throwing those exceptions.

Copy link
Member

@harshit-gangal harshit-gangal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 11 of 12 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @systay and @sougou)


java/jdbc/src/test/java/io/vitess/jdbc/VitessConnectionTest.java, line 40 at r1 (raw file):

import static org.junit.Assert.*;
import static org.mockito.Mockito.*;
 

explicitly import the classes / methods.


java/jdbc/src/test/java/io/vitess/jdbc/VitessStatementTest.java, line 48 at r1 (raw file):

import static org.junit.Assert.*;
import static org.mockito.Matchers.*;
import static org.mockito.Mockito.verify;

explicitly import the classes / methods.

Signed-off-by: Andres Taylor <[email protected]>
@sougou sougou requested a review from mpawliszyn December 15, 2018 22:23
@sougou
Copy link
Contributor

sougou commented Dec 15, 2018

@mpawliszyn: please review.

@mpawliszyn mpawliszyn merged commit a3143fe into vitessio:master Dec 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants