-
-
Notifications
You must be signed in to change notification settings - Fork 76
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
Merge PSQLRow into PostgresRow #219
Merge PSQLRow into PostgresRow #219
Conversation
8093749
to
55a2c0d
Compare
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.
LGTM, just a couple notes
|
||
extension PostgresRow: Equatable { | ||
public static func ==(lhs: Self, rhs: Self) -> Bool { | ||
lhs.data == rhs.data && lhs.columns == rhs.columns |
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.
Is the lookupTable
a deliberate omission here? (I have no objection if so, just double-checking.)
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.
Yes. Cause it is purely derived from the columns metadata.
} | ||
|
||
extension PostgresRow { | ||
// TODO: Remove this function. Only here to keep the tests running as of today. |
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.
Is it possible to move these methods into the tests target, given this status?
55a2c0d
to
635d2a5
Compare
635d2a5
to
bdff71b
Compare
Codecov Report
@@ Coverage Diff @@
## main #219 /- ##
==========================================
Coverage 39.62% 43.36% 3.73%
==========================================
Files 116 115 -1
Lines 9271 9327 56
==========================================
Hits 3674 4045 371
Misses 5597 5282 -315
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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 love cleanups ❤️ More, more! 🙃
# Conflicts: # Sources/PostgresNIO/Connection/PostgresConnection Database.swift # Sources/PostgresNIO/New/PSQLRow-multi-decode.swift # Sources/PostgresNIO/New/PSQLRow.swift # Tests/IntegrationTests/PSQLIntegrationTests.swift
# Conflicts: # Sources/PostgresNIO/New/PostgresRowSequence.swift
# Conflicts: # Sources/PostgresNIO/Connection/PostgresConnection Database.swift
# Conflicts: # Sources/PostgresNIO/Connection/PostgresConnection.swift # Sources/PostgresNIO/Postgres PSQLCompat.swift
Okay, this was already approved, but I think now it is finally ready... Just to make everyone aware, the important bits are:
|
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.
Still love it - let's keep it going! 👍 🎉
Okay, this PR has some meat in it! As it changes the existing PostgresRow quite significantly. We can do this non API breaking, however we may break some users expectations. However we can make those explicit with deprecations.
Motivation
Currently PostgresRow creates an internal array for every row to allow random access lookup for cells in the row. However I suspect this to not be always necessary. For this reason I propose changing PostgresRow to work like PSQLRow has worked so far. To allow dependent software that depends on the RandomAccessCollection lookups to continue to work correctly we introduce a PostgresRandomAccessRow. This one can be created from a PostgresRow and has all nice O(1) behaviors.
Changes
Result