self-review

crates/re_types_builder/build.rs

@@ -50,6 +50,8 @@ fn main() {
         }
     }
 
+    // NOTE: This requires `flatc` to be in $PATH, but only for contributors, not end users.
+    // Even for contributors, `flatc` won't be needed unless they edit some of the .fbs files.
     let sh = Shell::new().unwrap();
     cmd!(
         sh,
@@ -60,8 +62,8 @@ fn main() {
 
     // NOTE: We're purposefully ignoring the error here.
     //
-    // In the very unlikely chance that the user doesn't have `rustfmt` in their $PATH, there's
-    // still no good reason to fail the build.
+    // In the very unlikely chance that the user doesn't have the `fmt` component installed,
+    // there's still no good reason to fail the build.
     //
     // The CI will catch the unformatted file at PR time and complain appropriately anyhow.
     cmd!(sh, "cargo fmt").run().ok();

crates/re_types_builder/src/arrow_registry.rs

@@ -8,7 +8,6 @@ use crate::{ElementType, Object, Type};
 
 // ---
 
-// TODO(cmc): find a way to extract attr name constants directly from the IDL definitions
 pub const ARROW_ATTR_TRANSPARENT: &str = "arrow.attr.transparent";
 pub const ARROW_ATTR_SPARSE_UNION: &str = "arrow.attr.sparse_union";
 
@@ -56,7 +55,6 @@ impl ArrowRegistry {
 
     fn arrow_datatype_from_object(&self, obj: &Object) -> LazyDatatype {
         let is_struct = obj.is_struct();
-
         let is_transparent = obj.try_get_attr::<String>(ARROW_ATTR_TRANSPARENT).is_some();
         let num_fields = obj.fields.len();
 
@@ -170,6 +168,9 @@ impl ArrowRegistry {
 // --- Field ---
 
 /// A yet-to-be-resolved [`arrow2::datatypes::Field`].
+///
+/// Type resolution is a two-pass process as we first need to register all existing types before we
+/// can denormalize their definitions into their parents.
 #[derive(Debug, Clone, Eq, PartialEq, Hash)]
 pub struct LazyField {
     /// Its name

crates/re_types_builder/src/codegen/mod.rs

@@ -15,7 +15,6 @@ pub trait CodeGenerator {
 pub const AUTOGEN_WARNING: &str =
     "NOTE: This file was autogenerated by re_types_builder; DO NOT EDIT.";
 
-// TODO(cmc): find a way to extract attr name constants directly from the IDL definitions
 pub const RERUN_ATTR_COMPONENT_REQUIRED: &str = "rerun.attr.component_required";
 pub const RERUN_ATTR_COMPONENT_RECOMMENDED: &str = "rerun.attr.component_recommended";
 pub const RERUN_ATTR_COMPONENT_OPTIONAL: &str = "rerun.attr.component_optional";

crates/re_types_builder/src/codegen/python.rs

@@ -17,7 +17,6 @@ use crate::{
 // NOTE: `rerun2` while we figure out how to integrate back into the main SDK.
 const MODULE_NAME: &str = "rerun2";
 
-// TODO(cmc): find a way to extract attr name constants directly from the IDL definitions
 pub const ATTR_TRANSPARENT: &str = "python.attr.transparent";
 pub const ATTR_ALIASES: &str = "python.attr.aliases";
 pub const ATTR_ARRAY_ALIASES: &str = "python.attr.array_aliases";
@@ -440,7 +439,7 @@ impl QuotedObject {
 // --- Code generators ---
 
 fn quote_module_prelude() -> String {
-    // NOTE: All the extraneous stull will be cleaned up courtesy of `ruff`.
+    // NOTE: All the extraneous stuff will be cleaned up courtesy of `ruff`.
     unindent::unindent(
         r#"
         from __future__ import annotations
@@ -722,7 +721,6 @@ fn quote_type_from_element_type(typ: &ElementType) -> String {
         ElementType::Object(fqname) => {
             let (from, class) = fqname.rsplit_once('.').unwrap_or(("", fqname.as_str()));
             if from.starts_with("rerun.datatypes") {
-                // NOTE: Only need the class name, pre-generated import clause takes care of the rest.
                 format!("datatypes.{class}")
             } else if from.starts_with("rerun.components") {
                 format!("components.{class}")
@@ -808,7 +806,7 @@ fn quote_arrow_support_from_obj(arrow_registry: &ArrowRegistry, obj: &Object) ->
                                 many_aliases={many_aliases},
                                 arrow={arrow},
                             )
-            "#
+                "#
             ))
         }
         ObjectKind::Archetype => String::new(),
@@ -936,11 +934,7 @@ fn quote_arrow_datatype(datatype: &DataType) -> String {
                 .join(", ");
             format!("pa.struct([{fields}])")
         }
-        DataType::Extension(_, datatype, _) => {
-            // TODO(cmc): not sure we need all that for the python backend since we already
-            // do the wrapping trick...?
-            quote_arrow_datatype(datatype)
-        }
+        DataType::Extension(_, datatype, _) => quote_arrow_datatype(datatype),
         _ => unimplemented!("{datatype:#?}"), // NOLINT
     }
 }

crates/re_types_builder/src/codegen/rust.rs

@@ -18,7 +18,6 @@ use crate::{
 
 // ---
 
-// TODO(cmc): find a way to extract attr name constants directly from the IDL definitions
 pub const ATTR_DERIVE: &str = "rust.attr.derive";
 pub const ATTR_REPR: &str = "rust.attr.repr";
 pub const ATTR_TUPLE_STRUCT: &str = "rust.attr.tuple_struct";
@@ -149,7 +148,7 @@ fn quote_objects(
         for module in mods.keys() {
             code.push_str(&format!("mod {module};\n"));
 
-            // NOTE: detect if someone manually created an extension file, and automatically
+            // Detect if someone manually created an extension file, and automatically
             // import it if so.
             let mut ext_path = out_path.join(format!("{module}_ext"));
             ext_path.set_extension("rs");
@@ -230,7 +229,7 @@ impl QuotedObject {
                 typ: _,
                 attrs: _,
                 required,
-                // TODO(cmc): support for deprecation notices
+                // TODO(#2366): support for deprecation notices
                 deprecated: _,
             } = field;
 
@@ -356,7 +355,8 @@ fn quote_doc_from_docs(docs: &Docs) -> String {
 /// Returns type name as string and whether it was force unwrapped.
 fn quote_field_type_from_field(field: &ObjectField, unwrap: bool) -> (String, bool) {
     let mut unwrapped = false;
-    let typ = match &field.typ {
+    let typ = &field.typ;
+    let typ = match typ {
         Type::UInt8 => "u8".to_owned(),
         Type::UInt16 => "u16".to_owned(),
         Type::UInt32 => "u32".to_owned(),
@@ -366,10 +366,9 @@ fn quote_field_type_from_field(field: &ObjectField, unwrap: bool) -> (String, bo
         Type::Int32 => "i32".to_owned(),
         Type::Int64 => "i64".to_owned(),
         Type::Bool => "bool".to_owned(),
-        Type::Float16 => unimplemented!("ResolvedType::Float16"), // NOLINT
+        Type::Float16 => unimplemented!("{typ:#?}"), // NOLINT
         Type::Float32 => "f32".to_owned(),
         Type::Float64 => "f64".to_owned(),
-        // TODO(cmc): ref for deserialization?
         Type::String => "String".to_owned(),
         Type::Array { elem_type, length } => {
             let typ = quote_type_from_element_type(elem_type);
@@ -406,10 +405,9 @@ fn quote_type_from_element_type(typ: &ElementType) -> String {
         ElementType::Int32 => "i32".to_owned(),
         ElementType::Int64 => "i64".to_owned(),
         ElementType::Bool => "bool".to_owned(),
-        ElementType::Float16 => unimplemented!("ResolvedType::Float16"), // NOLINT
+        ElementType::Float16 => unimplemented!("{typ:#?}"), // NOLINT
         ElementType::Float32 => "f32".to_owned(),
         ElementType::Float64 => "f64".to_owned(),
-        // TODO(cmc): ref for deserialization?
         ElementType::String => "String".to_owned(),
         ElementType::Object(fqname) => fqname.replace('.', "::").replace("rerun", "crate"),
     }
@@ -536,6 +534,7 @@ fn quote_trait_impls_from_obj(arrow_registry: &ArrowRegistry, obj: &Object) -> S
 
                     #[allow(clippy::unimplemented)]
                     fn to_arrow_datatypes() -> Vec<arrow2::datatypes::DataType> {{
+                        // TODO(#2368): dump the arrow registry into the generated code
                         unimplemented!("query the registry for all fqnames"); // NOLINT
                     }}
                 }}

crates/re_types_builder/src/lib.rs

@@ -61,6 +61,8 @@
 //!
 //! Make sure to test the behavior of its output though: `re_types`!
 
+// TODO(#2365): support for external IDL definitions
+
 // ---
 
 // NOTE: Official generated code from flatbuffers; ignore _everything_.
@@ -146,7 +148,6 @@ pub fn compile_binary_schemas(
 /// - `include_dir_path`: path to the root directory of the fbs definition tree.
 /// - `output_crate_path`: path to the root of the output crate.
 /// - `entrypoint_path`: path to the root file of the fbs definition tree.
-/// - `source_hash`: optional sha256 hash of the source definition files.
 ///
 /// E.g.:
 /// ```no_run