-
Notifications
You must be signed in to change notification settings - Fork 209
CIDR Support in vertx-pg-client #1418
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
| case 2: | ||
| // IPV4 | ||
| try { | ||
| address = Inet4Address.getByAddress(data); |
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.
Actually InetAddress.getByAddress
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.
Addressed.
vertx-pg-client/src/main/java/io/vertx/pgclient/impl/codec/DataTypeCodec.java
Outdated
Show resolved
Hide resolved
vertx-pg-client/src/main/java/io/vertx/pgclient/impl/codec/DataTypeCodec.java
Outdated
Show resolved
Hide resolved
vietj
left a comment
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.
a few concerns to address
|
Addressed the previous comments. Please re-review it. @vietj |
|
can you squash your commits in a single one ? |
|
Sure ,doing it |
|
@vietj Hi sorry for delayed response, I have made the changes and squashed the commits into single commit. Kindly have a look at it |
|
Hi @vietj , any review comments on the PR? |
vietj
left a comment
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 cannot see any tests with a real database and the CIDR type, we need to have at least a test that read / write CIDR objects to a test container postgresql instance. Look at how other specific types are tested.
|
@vietj I have added test cases dependent on a real database. I have tested them with pg embedded version 12 as previous versions didn't have CIDR support. Please review and provide input. |
|
great feature thanks @AyanKoche |
Motivation: CIDR Data type support in vertx-pg-client
Explain here the context, and why you're making that change, what is the problem you're trying to solve.
There are several data types supported by vertx-pg-client but CIDR was missing implementation. As the project I am working on requires that support, contributing it is the better way.
Changes:
Conformance:
You should have signed the Eclipse Contributor Agreement as explained in https://github.com/eclipse/vert.x/blob/master/CONTRIBUTING.md
Please also make sure you adhere to the code style guidelines: https://github.com/vert-x3/wiki/wiki/Vert.x-code-style-guidelines