Replace model.QRecord with []qvalue.QValue #1142
Conversation
serprex
force-pushed
the
remove-qrecord
branch
from
774feec to
c78ff84
Compare
Also remove NumRecords from QRecordBatch; use `len(batch.Records)`
serprex
force-pushed
the
remove-qrecord
branch
from
c78ff84 to
64f13bf
Compare
| @@ -130,7 +130,7 @@ func (qe *QRepQueryExecutor) ProcessRows( | |||
| fieldDescriptions []pgconn.FieldDescription, | |||
| ) (*model.QRecordBatch, error) { | |||
| // Initialize the record slice | |||
| records := make([]model.QRecord, 0) | |||
| records := make([][]qvalue.QValue, 0) | |||
There was a problem hiding this comment.
will help readability if we do something like this
type QValueArr []qvalue.QValue
There was a problem hiding this comment.
I disagree, moving [] to the end as Arr adds little & requires wrapping the type while passing it around, when we just want []qvalue.QValue anyways
I did consider putting type QRecord []qvalue.QValue in model, but seems like it obfuscates the truth. I'm generally annoyed when I'm using a library & it turns out they've wrapped []T into some other type. I'd go as far as to say I'd prefer a type QRecord struct { Values []qvalue.QValue } over a type alias when it comes to wrapping slice types
There was a problem hiding this comment.
For me it's that double slices read weird, and having an alias for that would help. Fair point on requiring type wrapping though, would a type alias work better? https://stackoverflow.com/questions/61247864/what-is-the-difference-between-type-alias-and-type-definition-in-go
Approving anyway as this is primarily a stylistic difference.
There was a problem hiding this comment.
Type alias is type wrapping, it's specifically what I want to avoid since it makes a type that you can use slice operators on without being an explicit slice type, so you'll end up casting it to slice when you want to do other things like call functions from slices on it
Double slice is indeed awkward, but it let's someone know they should get used to dealing with nested slices since signature or not that's what they're dealing with
No description provided.