refactor: resolve clippy warnings and apply idiomatic rust patterns#108
refactor: resolve clippy warnings and apply idiomatic rust patterns#108marlon-costa-dc wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: marlon-costa-dc The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@marlon-costa-dc Thanks for your contribution. Please submit with DCO, see the contributing guide https://github.com/milvus-io/milvus/blob/master/CONTRIBUTING.md#developer-certificate-of-origin-dco. |
|
@marlon-costa-dc Please associate the related issue to the body of your Pull Request. (eg. “issue: #187”) |
| schema::FieldSchema { | ||
| field_id: 0, | ||
| name: fld.name.into(), | ||
| name: fld.name.clone(), |
There was a problem hiding this comment.
fld.name.clone() performs a needless heap allocation. fld is an owned parameter consumed by this From impl, and name is only used here — it can be moved directly. This also contradicts the PR's stated goal of idiomatic Rust patterns, and is inconsistent with line 415 where description: fld.description already uses a direct move.
| @@ -1,4 +1,5 @@ | |||
| use std::borrow::Cow; | |||
| use std::convert::TryFrom; | |||
There was a problem hiding this comment.
The project uses edition = "2021" (Cargo.toml), which includes TryFrom in the prelude. This import is unnecessary — src/schema.rs in the same PR uses try_from without it. The same redundant import also appears at src/types.rs:1. In a PR that aims to clean up clippy warnings, adding superfluous imports is counterproductive.
yhmo
force-pushed
the
refactor/clippy-and-idiomatic-fixes
branch
from
9e298dc to
2b89375
Compare
yhmo
force-pushed
the
refactor/clippy-and-idiomatic-fixes
branch
from
2b89375 to
89c03e1
Compare
There was a problem hiding this comment.
Pull request overview
This PR focuses on addressing clippy warnings and making several small refactors to align the SDK with more idiomatic Rust patterns (e.g., matches!, field init shorthand, and derived Default).
Changes:
- Refactors a number of boolean / match-based checks to use
matches!()and other idiomatic patterns. - Replaces several manual
Defaultimpls with#[derive(Default)], and simplifies struct initializations. - Adjusts protobuf enum decoding to use
DataType::try_from(...)and adds anis_empty()convenience method onFieldColumn.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/value.rs | Refactors check_dtype to use matches!() instead of a match with boolean returns. |
| src/types.rs | Switches enum decoding to DataType::try_from(...) (with a new import added). |
| src/schema.rs | Uses try_from for enum decoding; simplifies validate; tweaks schema conversions and builder logic. |
| src/partition.rs | Derives Default for options; simplifies request construction using ..Default::default(). |
| src/options.rs | Replaces manual Default impl with #[derive(Default)] for options. |
| src/mutate.rs | Derives Default for insert options; minor string-building tweaks in delete expression composition. |
| src/index/mod.rs | Uses field init shorthand in From<IndexDescription> for IndexInfo. |
| src/database.rs | Replaces manual Default impl with #[derive(Default)] for database options. |
| src/data.rs | Switches enum decoding to try_from; adds FieldColumn::is_empty(). |
| src/client.rs | Minor string formatting simplification (format! -> to_string). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fields: this.inner, | ||
| name: this.name, | ||
| description: this.description, | ||
| enable_dynamic_field: self.enable_dynamic_field, |
| schema::FieldSchema { | ||
| field_id: 0, | ||
| name: fld.name.into(), | ||
| name: fld.name.clone(), | ||
| is_primary_key: fld.is_primary, | ||
| description: fld.description, |
| self.value.len() / self.dim as usize | ||
| } | ||
|
|
||
| #[inline] | ||
| pub fn is_empty(&self) -> bool { | ||
| self.len() == 0 |
| @@ -1,4 +1,5 @@ | |||
| use std::borrow::Cow; | |||
| use std::convert::TryFrom; | |||
| use std::convert::TryFrom; | ||
|
|
| if i > 0 { | ||
| expr.push_str(","); | ||
| expr.push(','); | ||
| } | ||
| expr.push_str(v.as_str()); | ||
| } |
| (DataType::Int64, ValueVec::Long(values)) => { | ||
| for (i, v) in values.iter().enumerate() { | ||
| if i > 0 { | ||
| expr.push_str(","); | ||
| expr.push(','); | ||
| } | ||
| expr.push_str(format!("{}", v).as_str()); | ||
| } |
Summary
push_str(",")withpush(',').matches!()macro where appropriate.is_empty()method toFieldColumn.Defaultimplementations in favor of#[derive(Default)].