This is an automated email from the ASF dual-hosted git repository.
mgrigorov pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/avro-rs.git
The following commit(s) were added to refs/heads/main by this push:
new e2901d3 chore: Apply some suggestions by -Dclippy::pedantic (#514)
e2901d3 is described below
commit e2901d3cb644e96682b960c015b30b3475edb72e
Author: Martin Grigorov <[email protected]>
AuthorDate: Tue Mar 31 10:11:25 2026 +0300
chore: Apply some suggestions by -Dclippy::pedantic (#514)
* chore: Apply some suggestions by -Dclippy::pedantic
`-Dclippy::pedantic` is not added to test-lang-rust-clippy.yml because
not all sugestions could be applied at the moment.
Added `-Dclippy::cargo` to CI. `multiple_crate_versions` is needed
because at the moment there are two versions of `heck` and `hashbrown`
coming as transitive dependencies
* Simplify clear_log_messages() helper method
Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
* Move clippy settings to Cargo.toml
Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
* Simplify the docstrings for logging related helper methods
Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
* Improve assert_not_logged() to check all messages, not just the last one
Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
* Add the clippy settings which were fixed to make clippy::pedantic happy
But listing them explicitly here finds many more issues.
Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
* Fix docstring tests
Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
* Disable the extra lints now
https://github.com/apache/avro-rs/actions/runs/23512928423/job/68438311919?pr=514
They will be re-enabled once #512 is merged. To avoid merge conflicts by
editing many files in two PRs
Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
* Improve assert_logged() to remove only one matching log message, not all
Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
* Simplify by using `impl AsRef<[]>`
Suggested-by: Kriskras99
* Remove `#[inline]`s
Clippy asked to add them. Kriskras99 asked to remove them.
* Update the docstring for assert_not_logged
Co-authored-by: Kriskras99 <[email protected]>
* Hide imports from the user
Co-authored-by: Kriskras99 <[email protected]>
---------
Signed-off-by: Martin Tzvetanov Grigorov <[email protected]>
Co-authored-by: Kriskras99 <[email protected]>
---
.github/workflows/test-lang-rust-clippy.yml | 8 +--
Cargo.toml | 9 +++
avro/src/serde/ser_schema/mod.rs | 16 +++---
avro_derive/build.rs | 2 +-
avro_derive/src/attributes/avro.rs | 14 ++---
avro_derive/src/attributes/mod.rs | 6 +-
avro_derive/src/case.rs | 8 ++-
avro_derive/src/enums/mod.rs | 2 +-
avro_derive/src/enums/plain.rs | 4 +-
avro_derive/src/lib.rs | 43 ++++++++-------
avro_test_helper/src/data.rs | 2 +-
avro_test_helper/src/lib.rs | 18 +++---
avro_test_helper/src/logger.rs | 85 +++++++++++++++++++++--------
13 files changed, 133 insertions(+), 84 deletions(-)
diff --git a/.github/workflows/test-lang-rust-clippy.yml
b/.github/workflows/test-lang-rust-clippy.yml
index aae3834..4992eb2 100644
--- a/.github/workflows/test-lang-rust-clippy.yml
+++ b/.github/workflows/test-lang-rust-clippy.yml
@@ -19,9 +19,9 @@ name: Rust Clippy Check
on:
workflow_dispatch:
push:
- branches: [ main ]
+ branches: [main]
pull_request:
- branches: [ main ]
+ branches: [main]
permissions:
contents: read
@@ -39,11 +39,11 @@ jobs:
strategy:
matrix:
rust:
- - 'stable'
+ - "stable"
steps:
- uses: actions/checkout@v6
- uses: dtolnay/rust-toolchain@29eef336d9b2848a0b548edc03f92a220660cdb8
# stable
with:
toolchain: ${{ matrix.rust }}
components: clippy
- - run: cargo clippy --all-features --all-targets
+ - run: cargo clippy --all-features --all-targets --workspace
diff --git a/Cargo.toml b/Cargo.toml
index 1d44064..d57ce73 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -60,3 +60,12 @@ clippy.doc_markdown = "warn"
#clippy.missing_errors_doc = "warn"
#clippy.missing_panics_doc = "warn"
rust.unexpected_cfgs = { level = "warn", check-cfg = ['cfg(nightly)'] }
+clippy.cargo = { level = "warn", priority = -1 }
+clippy.multiple_crate_versions = "allow"
+# clippy.needless_raw_string_hashes = "warn"
+# clippy.missing_panics_doc = "warn"
+# clippy.semicolon_if_nothing_returned = "warn"
+# clippy.manual_assert = "warn"
+# clippy.enum_glob_use = "warn"
+# clippy.needless_pass_by_value = "warn"
+# clippy.single_match_else = "warn"
diff --git a/avro/src/serde/ser_schema/mod.rs b/avro/src/serde/ser_schema/mod.rs
index fa9382f..6f29b47 100644
--- a/avro/src/serde/ser_schema/mod.rs
+++ b/avro/src/serde/ser_schema/mod.rs
@@ -1645,15 +1645,13 @@ impl<'s, W: Write> SchemaAwareWriteSerializer<'s, W> {
Schema::Union(union_schema) => {
for (i, variant_schema) in
union_schema.schemas.iter().enumerate() {
match variant_schema {
- Schema::Record(inner) => {
- if inner.fields.len() == len {
- encode_int(i as i32, &mut *self.writer)?;
- return self.serialize_tuple_struct_with_schema(
- name,
- len,
- variant_schema,
- );
- }
+ Schema::Record(inner) if inner.fields.len() == len => {
+ encode_int(i as i32, &mut *self.writer)?;
+ return self.serialize_tuple_struct_with_schema(
+ name,
+ len,
+ variant_schema,
+ );
}
Schema::Array(_) | Schema::Ref { name: _ } => {
encode_int(i as i32, &mut *self.writer)?;
diff --git a/avro_derive/build.rs b/avro_derive/build.rs
index 40e33af..e3b221c 100644
--- a/avro_derive/build.rs
+++ b/avro_derive/build.rs
@@ -18,7 +18,7 @@
//! Set the `nightly` cfg value on nightly toolchains.
//!
//! We would prefer to just do `#![rustversion::attr(nightly,
feature(proc_macro_diagnostic)]`
-//! but that's currently not possible, see
<https://github.com/dtolnay/rustversion/issues/8>
+//! but that's currently not possible, see
<https://github.com/dtolnay/rustversion/issues/8>.
#[rustversion::nightly]
fn main() {
diff --git a/avro_derive/src/attributes/avro.rs
b/avro_derive/src/attributes/avro.rs
index eb11b9e..e0a1433 100644
--- a/avro_derive/src/attributes/avro.rs
+++ b/avro_derive/src/attributes/avro.rs
@@ -66,14 +66,14 @@ impl ContainerAttributes {
span,
r#"`#[avro(name = "...")]` is deprecated."#,
r#"Use `#[serde(rename = "...")]` instead."#,
- )
+ );
}
if self.rename_all != RenameRule::None {
super::warn(
span,
r#"`#[avro(rename_all = "..")]` is deprecated"#,
r#"Use `#[serde(rename_all = "..")]` instead"#,
- )
+ );
}
}
}
@@ -98,7 +98,7 @@ impl VariantAttributes {
span,
r#"`#[avro(rename = "..")]` is deprecated"#,
r#"Use `#[serde(rename = "..")]` instead"#,
- )
+ );
}
}
}
@@ -177,28 +177,28 @@ impl FieldAttributes {
span,
r#"`#[avro(alias = "..")]` is deprecated"#,
r#"Use `#[serde(alias = "..")]` instead"#,
- )
+ );
}
if self.rename.is_some() {
super::warn(
span,
r#"`#[avro(rename = "..")]` is deprecated"#,
r#"Use `#[serde(rename = "..")]` instead"#,
- )
+ );
}
if self.skip {
super::warn(
span,
"`#[avro(skip)]` is deprecated",
"Use `#[serde(skip)]` instead",
- )
+ );
}
if self.flatten {
super::warn(
span,
"`#[avro(flatten)]` is deprecated",
"Use `#[serde(flatten)]` instead",
- )
+ );
}
}
}
diff --git a/avro_derive/src/attributes/mod.rs
b/avro_derive/src/attributes/mod.rs
index 27c1cbc..66911d9 100644
--- a/avro_derive/src/attributes/mod.rs
+++ b/avro_derive/src/attributes/mod.rs
@@ -202,7 +202,7 @@ pub enum With {
impl With {
fn from_avro_and_serde(
avro: &avro::With,
- serde: &Option<String>,
+ serde: Option<&String>,
span: Span,
) -> Result<Self, syn::Error> {
match &avro {
@@ -327,7 +327,7 @@ impl FieldOptions {
"`#[serde(skip_serializing)]` and
`#[serde(skip_serializing_if)]` are incompatible with `#[avro(default =
false)]`"
));
}
- let with = match With::from_avro_and_serde(&avro.with, &serde.with,
span) {
+ let with = match With::from_avro_and_serde(&avro.with,
serde.with.as_ref(), span) {
Ok(with) => with,
Err(error) => {
errors.push(error);
@@ -389,7 +389,7 @@ fn darling_to_syn(e: darling::Error) -> Vec<syn::Error> {
fn warn(span: Span, message: &str, help: &str) {
proc_macro::Diagnostic::spanned(span.unwrap(), proc_macro::Level::Warning,
message)
.help(help)
- .emit()
+ .emit();
}
#[cfg(not(nightly))]
diff --git a/avro_derive/src/case.rs b/avro_derive/src/case.rs
index c958562..9f8eff9 100644
--- a/avro_derive/src/case.rs
+++ b/avro_derive/src/case.rs
@@ -21,7 +21,10 @@
use darling::FromMeta;
use syn::Lit;
-use self::RenameRule::*;
+use self::RenameRule::{
+ CamelCase, KebabCase, LowerCase, None, PascalCase, ScreamingKebabCase,
ScreamingSnakeCase,
+ SnakeCase, UpperCase,
+};
use std::fmt::{self, Debug, Display};
/// The different possible ways to change case of fields in a struct, or
variants in an enum.
@@ -111,7 +114,7 @@ impl RenameRule {
pub fn apply_to_field(self, field: &str) -> String {
match self {
None | LowerCase | SnakeCase => field.to_owned(),
- UpperCase => field.to_ascii_uppercase(),
+ UpperCase | ScreamingSnakeCase => field.to_ascii_uppercase(),
PascalCase => {
let mut pascal = String::new();
let mut capitalize = true;
@@ -131,7 +134,6 @@ impl RenameRule {
let pascal = PascalCase.apply_to_field(field);
pascal[..1].to_ascii_lowercase() + &pascal[1..]
}
- ScreamingSnakeCase => field.to_ascii_uppercase(),
KebabCase => field.replace('_', "-"),
ScreamingKebabCase =>
ScreamingSnakeCase.apply_to_field(field).replace('_', "-"),
}
diff --git a/avro_derive/src/enums/mod.rs b/avro_derive/src/enums/mod.rs
index e44f4a1..1511f65 100644
--- a/avro_derive/src/enums/mod.rs
+++ b/avro_derive/src/enums/mod.rs
@@ -24,7 +24,7 @@ use syn::{Attribute, DataEnum, Fields, Meta};
/// Generate a schema definition for a enum.
pub fn get_data_enum_schema_def(
container_attrs: &NamedTypeOptions,
- data_enum: DataEnum,
+ data_enum: &DataEnum,
ident_span: Span,
) -> Result<TokenStream, Vec<syn::Error>> {
if data_enum.variants.iter().all(|v| Fields::Unit == v.fields) {
diff --git a/avro_derive/src/enums/plain.rs b/avro_derive/src/enums/plain.rs
index 2832c08..4fb0adb 100644
--- a/avro_derive/src/enums/plain.rs
+++ b/avro_derive/src/enums/plain.rs
@@ -26,13 +26,13 @@ use syn::{DataEnum, Fields};
pub fn schema_def(
container_attrs: &NamedTypeOptions,
- data_enum: DataEnum,
+ data_enum: &DataEnum,
ident_span: Span,
) -> Result<TokenStream, Vec<syn::Error>> {
let doc = preserve_optional(container_attrs.doc.as_ref());
let enum_aliases = aliases(&container_attrs.aliases);
if data_enum.variants.iter().all(|v| Fields::Unit == v.fields) {
- let default_value = default_enum_variant(&data_enum, ident_span)?;
+ let default_value = default_enum_variant(data_enum, ident_span)?;
let default = preserve_optional(default_value);
let mut symbols = Vec::new();
for variant in &data_enum.variants {
diff --git a/avro_derive/src/lib.rs b/avro_derive/src/lib.rs
index 96f997e..d63ed8f 100644
--- a/avro_derive/src/lib.rs
+++ b/avro_derive/src/lib.rs
@@ -68,16 +68,16 @@ fn derive_avro_schema(input: DeriveInput) ->
Result<TokenStream, Vec<syn::Error>
let (schema_def, record_fields) =
get_struct_schema_def(&named_type_options, data_struct,
input.ident.span())?;
(
- handle_named_schemas(named_type_options.name, schema_def),
+ handle_named_schemas(&named_type_options.name,
&schema_def),
record_fields,
)
};
Ok(create_trait_definition(
- input.ident,
+ &input.ident,
&input.generics,
- get_schema_impl,
- get_record_fields_impl,
- named_type_options.default,
+ &get_schema_impl,
+ &get_record_fields_impl,
+ &named_type_options.default,
))
}
syn::Data::Enum(data_enum) => {
@@ -89,14 +89,14 @@ fn derive_avro_schema(input: DeriveInput) ->
Result<TokenStream, Vec<syn::Error>
)]);
}
let schema_def =
- get_data_enum_schema_def(&named_type_options, data_enum,
input.ident.span())?;
- let inner = handle_named_schemas(named_type_options.name,
schema_def);
+ get_data_enum_schema_def(&named_type_options, &data_enum,
input.ident.span())?;
+ let inner = handle_named_schemas(&named_type_options.name,
&schema_def);
Ok(create_trait_definition(
- input.ident,
+ &input.ident,
&input.generics,
- inner,
- quote! { ::std::option::Option::None },
- named_type_options.default,
+ &inner,
+ "e! { ::std::option::Option::None },
+ &named_type_options.default,
))
}
syn::Data::Union(_) => Err(vec![syn::Error::new(
@@ -108,11 +108,11 @@ fn derive_avro_schema(input: DeriveInput) ->
Result<TokenStream, Vec<syn::Error>
/// Generate the trait definition with the correct generics
fn create_trait_definition(
- ident: Ident,
+ ident: &Ident,
generics: &Generics,
- get_schema_impl: TokenStream,
- get_record_fields_impl: TokenStream,
- field_default_impl: TokenStream,
+ get_schema_impl: &TokenStream,
+ get_record_fields_impl: &TokenStream,
+ field_default_impl: &TokenStream,
) -> TokenStream {
let (impl_generics, ty_generics, where_clause) = generics.split_for_impl();
quote! {
@@ -134,7 +134,7 @@ fn create_trait_definition(
}
/// Generate the code to check `named_schemas` if this schema already exist
-fn handle_named_schemas(full_schema_name: String, schema_def: TokenStream) ->
TokenStream {
+fn handle_named_schemas(full_schema_name: &str, schema_def: &TokenStream) ->
TokenStream {
quote! {
let name =
::apache_avro::schema::Name::new_with_enclosing_namespace(#full_schema_name,
enclosing_namespace).expect(concat!("Unable to parse schema name ",
#full_schema_name));
if named_schemas.contains(&name) {
@@ -448,15 +448,16 @@ fn type_to_field_default_expr(ty: &Type) ->
Result<TokenStream, Vec<syn::Error>>
}
/// Stolen from serde
-fn to_compile_errors(errors: Vec<syn::Error>) -> proc_macro2::TokenStream {
- let compile_errors = errors.iter().map(syn::Error::to_compile_error);
+fn to_compile_errors(errors: impl AsRef<[syn::Error]>) ->
proc_macro2::TokenStream {
+ let compile_errors =
errors.as_ref().iter().map(syn::Error::to_compile_error);
quote!(#(#compile_errors)*)
}
fn preserve_optional(op: Option<impl quote::ToTokens>) -> TokenStream {
- match op {
- Some(tt) => quote! {::std::option::Option::Some(#tt.into())},
- None => quote! {::std::option::Option::None},
+ if let Some(tt) = op {
+ quote! {::std::option::Option::Some(#tt.into())}
+ } else {
+ quote! {::std::option::Option::None}
}
}
diff --git a/avro_test_helper/src/data.rs b/avro_test_helper/src/data.rs
index 3713e83..18a4310 100644
--- a/avro_test_helper/src/data.rs
+++ b/avro_test_helper/src/data.rs
@@ -37,7 +37,7 @@ pub const PRIMITIVE_EXAMPLES: &[(&str, bool)] = &[
(r#""double""#, true),
(r#"{"type": "double"}"#, true),
(r#""true""#, false),
- (r#"true"#, false),
+ ("true", false),
(r#"{"no_type": "test"}"#, false),
(r#"{"type": "panther"}"#, false),
];
diff --git a/avro_test_helper/src/lib.rs b/avro_test_helper/src/lib.rs
index 9b5248d..686d96f 100644
--- a/avro_test_helper/src/lib.rs
+++ b/avro_test_helper/src/lib.rs
@@ -15,9 +15,14 @@
// specific language governing permissions and limitations
// under the License.
+pub mod data;
+pub mod logger;
+
+use core::any::type_name;
+use core::cell::RefCell;
+use core::fmt::{Debug, Display};
#[cfg(not(target_arch = "wasm32"))]
use ctor::{ctor, dtor};
-use std::cell::RefCell;
thread_local! {
// The unit tests run in parallel
@@ -26,9 +31,6 @@ thread_local! {
pub(crate) static LOG_MESSAGES: RefCell<Vec<String>> = const {
RefCell::new(Vec::new()) };
}
-pub mod data;
-pub mod logger;
-
#[cfg(not(target_arch = "wasm32"))]
#[ctor]
fn before_all() {
@@ -56,13 +58,13 @@ pub struct TestError;
/// A converter of any error into [`TestError`].
///
/// It is used to print better error messages in the tests.
-/// Borrowed from
<https://bluxte.net/musings/2023/01/08/improving_failure_messages_rust_tests/>
+/// Borrowed from
<https://bluxte.net/musings/2023/01/08/improving_failure_messages_rust_tests/>.
// The Display bound is needed so that the `From` implementation doesn't
// apply to `TestError` itself.
-impl<Err: std::fmt::Display + std::fmt::Debug> From<Err> for TestError {
+impl<Err: Display + Debug> From<Err> for TestError {
#[track_caller]
fn from(err: Err) -> Self {
- panic!("{}: {:?}", std::any::type_name::<Err>(), err);
+ panic!("{}: {:?}", type_name::<Err>(), err);
}
}
@@ -71,4 +73,4 @@ pub type TestResult = Result<(), TestError>;
/// Does nothing. Just loads the crate.
/// Should be used in the integration tests, because they do not use
[dev-dependencies]
/// and do not auto-load this crate.
-pub fn init() {}
+pub const fn init() {}
diff --git a/avro_test_helper/src/logger.rs b/avro_test_helper/src/logger.rs
index 738cdee..e3d13ab 100644
--- a/avro_test_helper/src/logger.rs
+++ b/avro_test_helper/src/logger.rs
@@ -26,7 +26,6 @@ struct TestLogger {
}
impl Log for TestLogger {
- #[inline]
fn enabled(&self, _metadata: &Metadata) -> bool {
true
}
@@ -53,47 +52,85 @@ fn test_logger() -> &'static TestLogger {
})
}
+/// Clears all log messages of this thread.
pub fn clear_log_messages() {
- LOG_MESSAGES.with(|msgs| match msgs.try_borrow_mut() {
- Ok(mut log_messages) => log_messages.clear(),
- Err(err) => panic!("Failed to clear log messages: {err:?}"),
- });
+ LOG_MESSAGES.with_borrow_mut(Vec::clear);
}
+/// Asserts that the message is not in the log for this thread.
+///
+/// # Panics
+/// Will panic if the provided message is an exact match for any message in
the log.
+///
+/// # Example
+/// ```should_panic
+/// # use apache_avro_test_helper::logger::assert_not_logged;
+/// #
+/// log::error!("Unexpected Error");
+///
+/// // This will not panic as it is not an exact match
+/// assert_not_logged("No Unexpected Error");
+///
+/// // This will panic
+/// assert_not_logged("Unexpected Error");
+/// ```
#[track_caller]
pub fn assert_not_logged(unexpected_message: &str) {
- LOG_MESSAGES.with(|msgs| match msgs.borrow().last() {
- Some(last_log) if last_log == unexpected_message => {
- panic!("The following log message should not have been logged:
'{unexpected_message}'")
- }
- _ => (),
+ LOG_MESSAGES.with_borrow(|msgs| {
+ let is_logged = msgs.iter().any(|msg| msg == unexpected_message);
+ assert!(
+ !is_logged,
+ "The following log message should not have been logged:
'{unexpected_message}'"
+ );
});
}
+/// Asserts that the message has been logged and removes it from the log of
this thread.
+///
+/// # Panics
+/// Will panic if the message does not appear in the log.
+///
+/// # Example
+/// ```should_panic
+/// # use apache_avro_test_helper::logger::assert_logged;
+/// #
+/// log::error!("Something went wrong");
+/// log::info!("Something happened");
+/// log::error!("Something went wrong");
+///
+/// // This will not panic as the message was logged
+/// assert_logged("Something went wrong");
+///
+/// // This will not panic as the message was logged
+/// assert_logged("Something happened");
+///
+/// // This will not panic as the first call removed only the first matching
log message
+/// assert_logged("Something went wrong");
+///
+/// // This will panic as all matching log messages have been removed
+/// assert_logged("Something went wrong");
+/// ```
#[track_caller]
pub fn assert_logged(expected_message: &str) {
let mut deleted = false;
- LOG_MESSAGES.with(|msgs| {
- msgs.borrow_mut().retain(|msg| {
- if msg == expected_message {
- deleted = true;
- false
- } else {
- true
- }
- })
+ LOG_MESSAGES.with_borrow_mut(|msgs| {
+ if let Some(pos) = msgs.iter().position(|msg| msg == expected_message)
{
+ msgs.remove(pos);
+ deleted = true;
+ }
});
- if !deleted {
- panic!("Expected log message has not been logged:
'{expected_message}'");
- }
+ assert!(
+ deleted,
+ "Expected log message has not been logged: '{expected_message}'"
+ );
}
pub(crate) fn install() {
log::set_logger(test_logger())
- .map(|_| log::set_max_level(LevelFilter::Trace))
+ .map(|()| log::set_max_level(LevelFilter::Trace))
.map_err(|err| {
eprintln!("Failed to set the custom logger: {err:?}");
})
- .unwrap();
+ .expect("Failed to set the custom TestLogger");
}
