-
Notifications
You must be signed in to change notification settings - Fork 161
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
Fixes #45: Capture annotations in record schemas #66
Conversation
} | ||
|
||
final case class GenericRecord(override val structure: ListMap[String, Schema[_]]) extends Record[ListMap[String, _]] | ||
final case class GenericRecord(override val structure: Seq[Field[_]]) extends Record[ListMap[String, _]] |
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.
It's ok for Seq
to appear in contravariant position (as an input to a method), but it should not appear in covariant position (as an output from a method) because it provides no Big Oh guarantees on any of its methods.
Instead, I suggest we refactor this to be Chunk[Field[_]]
here and elsewhere, so that users have guaranteed O(1)
random access, etc.
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.
Makes sense
|
||
sealed trait Record[R] extends Schema[R] { | ||
def structure: Seq[Field[_]] | ||
def annotations: Seq[Any] = Nil |
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.
Ditto for all covariant uses of Seq
.
@thinkharderdev Thanks for your work on this! One step closer to the Ultimate Goal. |
Fixes #45
Capture annotations in
Record
schemas:Schema.Field[A]
type to encapsulate fields inRecord
schemaSeq[Schema.Field[_]]
instead ofListMap[String,Schema[_]]
. By usingListMap
we lose O(1) lookups anyway so this seemed like the better encoding.