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

Fix multiple array type mapping mistakes and add missing date and time array types #463

Merged
merged 4 commits into from
Mar 8, 2024

Conversation

gwynne
Copy link
Member

@gwynne gwynne commented Feb 25, 2024

  • Add the missing definitions and array mappings for OIDs 1182 (_date aka dateArray) and 1183 (_time aka timeArray).
  • Deprecate macaddr8Aray, renaming it to macaddr8Array.
  • Fix broken array mappings for macaddr8 and datemultirange.
  • Add missing array mappings for timestamp and tstzrange.

@gwynne gwynne added bug Something isn't working semver-minor Adds new public API. labels Feb 25, 2024
@codecov-commenter
Copy link

codecov-commenter commented Feb 25, 2024

Codecov Report

Attention: Patch coverage is 25.00000% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 61.70%. Comparing base (dc9caf8) to head (fc155c1).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #463      +/-   ##
==========================================
- Coverage   61.82%   61.70%   -0.12%     
==========================================
  Files         125      125              
  Lines       10019    10030      +11     
==========================================
- Hits         6194     6189       -5     
- Misses       3825     3841      +16     
Files Coverage Δ
...s/PostgresNIO/New/Data/Array+PostgresCodable.swift 72.00% <100.00%> (+0.28%) ⬆️
Sources/PostgresNIO/Data/PostgresDataType.swift 55.18% <20.00%> (-1.02%) ⬇️

... and 2 files with indirect coverage changes

@gwynne gwynne requested a review from 0xTim March 1, 2024 07:52
@gwynne
Copy link
Member Author

gwynne commented Mar 1, 2024

Note: There are no new tests because the effect of the missing mappings is not visible from within PostgresNIO itself; it affects PostgresKit due to hacks in that package which make atypical use of the array type mappings.

Copy link
Contributor

@MahdiBM MahdiBM left a comment

Choose a reason for hiding this comment

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

looks good

.github/workflows/test.yml Outdated Show resolved Hide resolved
Copy link
Collaborator

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

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

Would love if we could add an integration test, that writes and reads the new array types?

Sources/PostgresNIO/Data/PostgresDataType.swift Outdated Show resolved Hide resolved
Copy link
Collaborator

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

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

Mostly looks good to me! Please revert the changes that are not core of this pr.

Tests/IntegrationTests/PostgresNIOTests.swift Outdated Show resolved Hide resolved
Tests/IntegrationTests/Utilities.swift Outdated Show resolved Hide resolved
Tests/IntegrationTests/Utilities.swift Outdated Show resolved Hide resolved
Tests/IntegrationTests/Utilities.swift Outdated Show resolved Hide resolved
Tests/IntegrationTests/Utilities.swift Outdated Show resolved Hide resolved
…nd _time), fix typos in the `macaddr8Array` and `datemultirange` types, and add missing array mappings for `timestamp` and `tstzrange`.
@fabianfett fabianfett merged commit b6496eb into main Mar 8, 2024
12 of 13 checks passed
@fabianfett fabianfett deleted the add-missing-type-oids branch March 8, 2024 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working semver-minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants