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

added support for all native user data types PostgreSQL 15 #193

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

llucenic
Copy link
Contributor

Hello, I have added support for all native PostgreSQL user data types (ranges, multiranges, network, bitstrings, geometry, xml and text search vector/query), for now except for multi-dimensional arrays and user-defined types. In order to add support for OidType.PointArray I had to explicitely distinguish between Point[] and Polygon by defining a solid struct Point in dpq2.conv.geometry. Please review and incorporate the changes in the code base on your sole discretion.

src/dpq2/conv/net.d Outdated Show resolved Hide resolved
@denizzzka
Copy link
Owner

denizzzka commented Mar 19, 2023

@llucenic Thank you!

In order to add support for OidType.PointArray I had to explicitely distinguish between Point[] and Polygon by defining a solid struct Point in dpq2.conv.geometry

Such case is need to cover by integration test

@llucenic
Copy link
Contributor Author

Denis, I do not understand fully your point regarding integration test. Is it possible you kind of fix the tests yourself?

@denizzzka
Copy link
Owner

It is common practice to immediately implement a test with a PR, yep? Is there some problem with this?
Tests can be added into native_tests.d file.

Ľudovít Lučenič added 2 commits March 21, 2023 11:29
Signed-off-by: Ľudovít Lučenič <[email protected]>
Signed-off-by: Ľudovít Lučenič <[email protected]>
@llucenic
Copy link
Contributor Author

I have corrected the code, so that it passes at least some of the check jobs. However, the rest of the failures are beyond the scope of my PR.

At the moment, I cannot do much more. Maybe later, and I am not sure though the code I added follows your design of the library. Therefore I have asked you to add the tests yourself while integrating the changes into your code base.

@denizzzka
Copy link
Owner

Maybe implementation of ranges as a separate PR would be the best solution. Otherwise this PR is too big.

@denizzzka
Copy link
Owner

denizzzka commented Mar 21, 2023

However, the rest of the failures are beyond the scope of my PR.

Yes, looks like this issues itroduced by new compilers and will be solved not in this repository.

But I am worry about lack of integration tests for ranges. This is fundamentally new structures, I would like to cover with tests at least one of each base type.

If it is too difficult feel free to copy and paste here your actual code what works with it and I'll try to implement test by myself

@llucenic
Copy link
Contributor Author

llucenic commented Mar 21, 2023

If integration tests for ranges are what makes you integrate this PR in your library, then I think I can provide them. Assuming that we avoid splitting this PR, which does not make much sense to me (as implementation of ranges is only code in conv/ranges.d).

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.

2 participants