Conversation
dannycjones
left a comment
dannycjones
left a comment
There was a problem hiding this comment.
I took a quick look, I think this is a much needed change, thanks @blackmwk
| .field_type(Type::Primitive(PrimitiveType::Long)) | ||
| .initial_default(Literal::Primitive(PrimitiveLiteral::Long(114514))) |
There was a problem hiding this comment.
nit: the type here changed - I think it's probably fine actually
| Arc::new(NestedField::new( | ||
| 1, | ||
| "col_float", | ||
| Type::Primitive(PrimitiveType::Float), | ||
| false, | ||
| )), | ||
| Arc::new(NestedField::new( | ||
| 2, | ||
| "col_string", | ||
| Type::Primitive(PrimitiveType::String), | ||
| false, | ||
| )), | ||
| Arc::new( | ||
| NestedField::new(1, "col_float", Type::Primitive(PrimitiveType::Float), false) | ||
| .expect("valid nested field"), | ||
| ), | ||
| Arc::new( | ||
| NestedField::new( | ||
| 2, | ||
| "col_string", | ||
| Type::Primitive(PrimitiveType::String), | ||
| false, | ||
| ) | ||
| .expect("valid nested field"), | ||
| ), |
There was a problem hiding this comment.
with these, I am wondering if it would be better to maintain these tests by constructing a simple vec, and then mapping with Arc::new and maybe even expect("all fields should be valid").
| pub(crate) fn rebuild(&self, id: i32, field_type: Type) -> Result<Self> { | ||
| Self::builder() | ||
| .id(id) | ||
| .name(self.name.clone()) | ||
| .required(self.required) | ||
| .field_type(field_type) | ||
| .doc_opt(self.doc.clone()) | ||
| .initial_default_opt(self.initial_default.clone()) | ||
| .write_default_opt(self.write_default.clone()) | ||
| .build() | ||
| } |
There was a problem hiding this comment.
Maybe some Rustdoc to explain in what scenario we may want to rebuild with a new ID or field type.
| .write_default(Literal::Primitive(PrimitiveLiteral::UInt128( | ||
| Uuid::parse_str("ec5911be-b0a7-458c-8438-c9a3e53cffae") | ||
| .unwrap() | ||
| .as_u128(), | ||
| ))) |
There was a problem hiding this comment.
I'm wondering if we can make some ergonomic improvements to some other APIs, by adopting Into<Literal> in write_default setter.
| .write_default(Literal::Primitive(PrimitiveLiteral::UInt128( | |
| Uuid::parse_str("ec5911be-b0a7-458c-8438-c9a3e53cffae") | |
| .unwrap() | |
| .as_u128(), | |
| ))) | |
| .write_default(PrimitiveLiteral::UInt128( | |
| Uuid::parse_str("ec5911be-b0a7-458c-8438-c9a3e53cffae") | |
| .unwrap() | |
| .as_u128(), | |
| )) |
This can be a follow-up, and it would be opt-in so new tests and changes could pick it up.
There was a problem hiding this comment.
The same for impl From<PrimitiveType> for Type
| /// Construct a required field. | ||
| pub fn required(id: i32, name: impl ToString, field_type: Type) -> Self { | ||
| pub fn required(id: i32, name: impl ToString, field_type: Type) -> Result<Self> { | ||
| Self::new(id, name, field_type, true) | ||
| } | ||
|
|
||
| /// Construct an optional field. | ||
| pub fn optional(id: i32, name: impl ToString, field_type: Type) -> Self { | ||
| pub fn optional(id: i32, name: impl ToString, field_type: Type) -> Result<Self> { | ||
| Self::new(id, name, field_type, false) | ||
| } |
There was a problem hiding this comment.
Would it be ergonomic here to return a builder, preconfigured as optional or required?
Then call sites are one of these:
let f0 = NestedField::required(0, "f0", PrimitiveType::Int).build().unwrap()
let f1 = NestedField::required(1, "f1", PrimitiveType::Int)
.write_default(Literal::Primitive(PrimitiveLiteral::Int(5)))
.build()
.unwrap()2 participants
Which issue does this PR close?
What changes are included in this PR?
Are these changes tested?
AI Disclosure
This PR was implemented with assistance from OpenAI Codex for code migration, test scaffolding, and verification. The implementation and generated changes were reviewed, and the full core test suite, workspace clippy checks, formatting checks, doctests, and public API checks were run locally. There are no known unresolved uncertainties.