Skip to content
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

resolve clippy::unwrap_used in re_types_core #8732

Merged

Conversation

brody4hire
Copy link
Contributor

Related

Part of: #6330

What

  • remove #![allow(clippy::unwrap_used)] & TODO comment from crates/store/re_types_core/src/lib.rs
  • add focused #[allow(clippy::unwrap_used)] directives to Tuid::from_arrow fn in crates/store/re_types_core/src/tuid.rs

RATIONALE:

Given the following comment in Tuid::from_arrow:

// NOTE: Unwrap is safe everywhere below, datatype is checked above.

I figured it would be easiest & cleanest to simply add the focused #[allow(clippy::unwrap_used)] directives into the Tuid::from_arrow fn. I did consider some other possible refactoring but decided against this.

Incidentally when I did try injecting a couple mutations, cargo test -p re_types_core --all-features continued to succeed with these:

diff --git a/crates/store/re_types_core/src/tuid.rs b/crates/store/re_types_core/src/tuid.rs
index 155340fac4..4fdbb8f696 100644
--- a/crates/store/re_types_core/src/tuid.rs
+++ b/crates/store/re_types_core/src/tuid.rs
@@ -68,15 +68,6 @@ impl Loggable for Tuid {
     }
 
     fn from_arrow(array: &dyn ::arrow::array::Array) -> crate::DeserializationResult<Vec<Self>> {
-        let expected_datatype = Self::arrow_datatype();
-        let actual_datatype = array.data_type();
-        if actual_datatype != &expected_datatype {
-            return Err(DeserializationError::datatype_mismatch(
-                expected_datatype,
-                actual_datatype.clone(),
-            ));
-        }
-
         // NOTE: Unwrap is safe everywhere below, datatype is checked above.
         // NOTE: We don't even look at the validity, our datatype says we don't care.
 
@@ -89,13 +80,6 @@ impl Loggable for Tuid {
         let (time_ns_index, inc_index) = {
             let mut time_ns_index = None;
             let mut inc_index = None;
-            for (i, field) in array.fields().iter().enumerate() {
-                if field.name() == "time_ns" {
-                    time_ns_index = Some(i);
-                } else if field.name() == "inc" {
-                    inc_index = Some(i);
-                }
-            }
             #[allow(clippy::unwrap_used)]
             (time_ns_index.unwrap(), inc_index.unwrap())
         };

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! Thanks for opening this pull request.

Because this is your first time contributing to this repository, make sure you've read our Contributor Guide and Code of Conduct.

Copy link
Member

@emilk emilk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@emilk emilk added 🧑‍💻 dev experience developer experience (excluding CI) 🚜 refactor Change the code, not the functionality exclude from changelog PRs with this won't show up in CHANGELOG.md labels Jan 20, 2025
@emilk emilk merged commit 343da4a into rerun-io:main Jan 22, 2025
33 of 35 checks passed
@brody4hire brody4hire deleted the resolve-unwrap-todo-in-re_types_core branch January 22, 2025 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧑‍💻 dev experience developer experience (excluding CI) exclude from changelog PRs with this won't show up in CHANGELOG.md 🚜 refactor Change the code, not the functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants