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

feat(data-types): CIDR, INET, MACADDR support for Postgres #9567

Merged
merged 2 commits into from Jun 20, 2018

Conversation

@gabegorelick
Copy link
Contributor

@gabegorelick gabegorelick commented Jun 19, 2018

Pull Request check-list

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

Adds support for Postgres network address types.

This is a continuation of work started by @tadman in #8154.

Closes #8154

@gabegorelick gabegorelick force-pushed the gabegorelick:pg-inet branch 3 times, most recently from d205b95 to 006d3a4 Jun 19, 2018
@codecov
Copy link

@codecov codecov bot commented Jun 19, 2018

Codecov Report

Merging #9567 into master will decrease coverage by 0.08%.
The diff coverage is 66.66%.

@@ -32,7 +32,7 @@
"semver": "^5.5.0",
"toposort-class": "^1.0.1",
"uuid": "^3.2.1",
"validator": "^10.0.0",
"validator": "^10.4.0",

This comment has been minimized.

@gabegorelick

gabegorelick Jun 20, 2018
Author Contributor

Needed for isIPRange.

@gabegorelick
Copy link
Contributor Author

@gabegorelick gabegorelick commented Jun 20, 2018

Tests are now passing (the appveyor failure looks to be an existing flaky test). Let me know if more tests make sense. I'll also try to add some relevant docs to model-definitions.md.

}
inherits(CIDR, BaseTypes.CIDR);

CIDR.parse = function parse(value) {

This comment has been minimized.

@sushantdhiman

sushantdhiman Jun 20, 2018
Contributor

No need to add parse if we are not modifying this value, I think just defining oids should be enough for all three of these types

This comment has been minimized.

@gabegorelick

gabegorelick Jun 20, 2018
Author Contributor

@gabegorelick gabegorelick force-pushed the gabegorelick:pg-inet branch from 006d3a4 to e27f7b5 Jun 20, 2018
@@ -647,6 +647,39 @@ module.exports = BaseTypes => {
array_oids: []
};

function CIDR() {

This comment has been minimized.

@sushantdhiman

sushantdhiman Jun 20, 2018
Contributor

Even these function definitions are not required, as there is no special handling required for Postgres initializing of these datatypes.

Just define OIDs for these types at the top with others

This comment has been minimized.

@gabegorelick

gabegorelick Jun 20, 2018
Author Contributor

Done.

@gabegorelick gabegorelick force-pushed the gabegorelick:pg-inet branch from e27f7b5 to a62889b Jun 20, 2018
@sushantdhiman sushantdhiman merged commit 53ec6af into sequelize:master Jun 20, 2018
2 of 4 checks passed
2 of 4 checks passed
codecov/patch 66.66% of diff hit (target 96.04%)
Details
codecov/project 95.96% (-0.09%) compared to a22b246
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gabegorelick gabegorelick deleted the gabegorelick:pg-inet branch Jun 20, 2018
@gabegorelick
Copy link
Contributor Author

@gabegorelick gabegorelick commented Jun 20, 2018

@sushantdhiman Thanks for shepherding this. Any chance it could get merged into v4?

@sushantdhiman
Copy link
Contributor

@sushantdhiman sushantdhiman commented Jun 20, 2018

@gabegorelick Please open a new PR targeting v4

@gabegorelick gabegorelick mentioned this pull request Jun 20, 2018
5 of 5 tasks complete
@gabegorelick
Copy link
Contributor Author

@gabegorelick gabegorelick commented Jun 20, 2018

Thanks @sushantdhiman. I opened #9571 for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants