Merge type PSQLFormat into PostgresFormat #212
Conversation
Codecov Report
@@ Coverage Diff @@
## main #212 +/- ##
==========================================
+ Coverage 40.44% 44.59% +4.14%
==========================================
Files 116 116
Lines 7771 7784 +13
==========================================
+ Hits 3143 3471 +328
+ Misses 4628 4313 -315
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
No objections whatsoever to this, just a minor question and one incredibly persnickety nitpick, neither is in any way blocking 👍
I don't think it was a mistake to add the new types at the time, incidentally - given the somewhat uneven quality of the old implementation, it mades sense to start with a type that could be freely tinkered with as needed rather than try to squeeze a new implementation into the old one's limitations. I do agree the extra type is obsolete at this point, though.
Also I just like the shorter type names 🤣
| @@ -14,6 +17,13 @@ public enum PostgresFormatCode: Int16, Codable, CustomStringConvertible { | |||
| } | |||
| } | |||
|
|
|||
| // TODO: The Codable conformance does not make any sense. Let's remove this with next major break. | |||
There was a problem hiding this comment.
IIRC, enums that get synthesized RawRepresentable by declaring an enumerator type automatically get Codable if the raw type is via RawRepresentable's conditional conformance anyway - was that what you were referring to with "does not make any sense?" (The phrasing is a little confusing to me; I'm wondering if I missed any additional issues.)
There was a problem hiding this comment.
"does not make sense" relates to the unlikeliness of users wanting to put a PostgresFormat into json/xml/yaml.
| /// Currently there a two wire formats supported: | ||
| /// - text | ||
| /// - binary | ||
| public enum PostgresFormat: Int16 { |
There was a problem hiding this comment.
I've gotten into the habit of tossing CaseIterable conformance on any enum I can unless there's a reason not to, on the theory that it tends to come in handy when testing, debugging, doing anything reflection-like, etc. Any reason not to put it on this one? (Other than it conceptually reducing to Bool unless Postgres's wire protocol someday comes up with additional formats 😆) I know it's overkill and there's no present need for it, I'm not gonna debate the point if you'd rather not add it (though, being me, I will probably grumble to myself just to be contrary 😉)
There was a problem hiding this comment.
I wouldn't oppose adding it... However everything that is public API, will make binaries larger as it can't be stripped as long as we don't have lto. That's why I refrain from adding auto synthesized conformances normally. If I don't need it they are not allowed to take up bytes.
Motivation
While rewriting lot's of PostgresNIO internals in the last year, I created new types that very closely match existing types. This was a mistake. I should have tried everything to reuse existing types, and change them as needed.
Changes
PSQLFormatPostgresFormatCodetoPostgresFormattypealias PostgresFormatCode = PostgresFormatwith deprecation to not break public APIResult