Kriskras99 commented on code in PR #514:
URL: https://github.com/apache/avro-rs/pull/514#discussion_r2980575893
##########
.github/workflows/test-lang-rust-clippy.yml:
##########
@@ -46,4 +46,4 @@ jobs:
with:
toolchain: ${{ matrix.rust }}
components: clippy
- - run: cargo clippy --all-features --all-targets
+ - run: cargo clippy --all-features --all-targets --workspace --
-Dclippy::cargo -Aclippy::multiple_crate_versions
Review Comment:
This should be configured in the root Cargo.toml:
```
clippy.cargo = { level = "warn", priority = -1 }
clippy.multiple_crate_versions = "allow"
```
And any pedantic lints you fixed completely should be added to the lint
table too, otherwise the code will regress because it isn't tested anymore
##########
avro_test_helper/src/logger.rs:
##########
@@ -53,13 +53,47 @@ fn test_logger() -> &'static TestLogger {
})
}
+/// Clears all log messages stored in the thread-local storage.
+///
+/// # Panics
+/// This function will panic if:
+/// - The thread-local `LOG_MESSAGES` is already borrowed and cannot
+/// be mutably borrowed.
+///
+/// The panic includes a debug representation of the error encountered.
Review Comment:
```suggestion
/// Clears all log messages of this thread.
```
The panic cannot happen, because at no point is the `RefCell` borrowed in
one part of the thread and used in another part of the thread
##########
avro_test_helper/src/logger.rs:
##########
@@ -53,13 +53,47 @@ fn test_logger() -> &'static TestLogger {
})
}
+/// Clears all log messages stored in the thread-local storage.
+///
+/// # Panics
+/// This function will panic if:
+/// - The thread-local `LOG_MESSAGES` is already borrowed and cannot
+/// be mutably borrowed.
+///
+/// The panic includes a debug representation of the error encountered.
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:?}"),
});
Review Comment:
```suggestion
LOG_MESSAGES.with_borrow_mut(Vec::clear)
```
##########
avro_test_helper/src/lib.rs:
##########
@@ -50,19 +52,21 @@ fn after_all() {
}
/// A custom error type for tests.
+#[non_exhaustive]
Review Comment:
This type is never actually created, so adding `non_exhaustive` doesn't
really do anything.
##########
avro_test_helper/src/data.rs:
##########
@@ -602,6 +602,7 @@ pub const LOCAL_TIMESTAMPMICROS_LOGICAL_TYPE: &[(&str,
bool)] = &[
),
];
+#[inline]
Review Comment:
```suggestion
```
Why add the inline? As this function is one function call it is most likely
already inlined and it is in test code, so we don't need to squeeze every
nanosecond of performance from it.
##########
avro_test_helper/src/logger.rs:
##########
@@ -53,13 +53,47 @@ fn test_logger() -> &'static TestLogger {
})
}
+/// Clears all log messages stored in the thread-local storage.
+///
+/// # Panics
+/// This function will panic if:
+/// - The thread-local `LOG_MESSAGES` is already borrowed and cannot
+/// be mutably borrowed.
+///
+/// The panic includes a debug representation of the error encountered.
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:?}"),
});
}
+/// Asserts that a specific log message has not been logged.
+///
+/// This function checks the most recently logged message from a thread-local
+/// storage and ensures that it does not match the provided
`unexpected_message`.
+/// If the message does match, the function will panic with an appropriate
error
+/// message indicating the unexpected log entry.
+///
+/// # Arguments
+/// - `unexpected_message`: A string slice that represents the log message
+/// that is not expected to be logged. If this message matches the last
+/// logged message, the function will panic.
+///
+/// # Panics
+/// - This function will panic if the `unexpected_message` matches the most
+/// recently logged message.
+///
+/// # Example
+/// ```ignore
+/// // Assume LOG_MESSAGES is set up to capture log messages.
+/// log_message("Unexpected Error");
+/// assert_not_logged("Unexpected Error");
+/// // This will panic with the message:
+/// // "The following log message should not have been logged: 'Unexpected
Error'"
+///
+/// assert_not_logged("Non-existent Message");
+/// // This will pass without issue since the message was not logged.
Review Comment:
```suggestion
/// Asserts that the message is not the last in the log for this thread.
///
/// # Panics
/// Will panic if the provided message is an exact match for the last log
message.
///
/// # Example
/// ```should_panic
/// log::error("Something went wrong");
/// log::error("Unexpected Error");
///
/// // This will not panic as it is not an exact match
/// assert_not_logged("No Unexpected Error");
///
/// // This will not panic as it is not the last log message
/// assert_not_logged("Something went wrong");
///
/// // This will panic
/// assert_not_logged("Unexpected Error");
/// ```
```
##########
avro_test_helper/src/lib.rs:
##########
@@ -50,19 +52,21 @@ fn after_all() {
}
/// A custom error type for tests.
+#[non_exhaustive]
#[derive(Debug)]
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]
+ #[inline]
Review Comment:
```suggestion
```
Why add the inline? As this function is one function call it is most likely
already inlined and it is in test code, so we don't need to squeeze every
nanosecond of performance from it.
##########
avro_test_helper/src/data.rs:
##########
@@ -629,6 +630,7 @@ pub fn examples() -> &'static Vec<(&'static str, bool)> {
})
}
+#[inline]
Review Comment:
```suggestion
```
Why add the inline? As this function is one function call it is most likely
already inlined and it is in test code, so we don't need to squeeze every
nanosecond of performance from it.
##########
avro_test_helper/src/lib.rs:
##########
@@ -71,4 +75,5 @@ 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() {}
+#[inline]
Review Comment:
```suggestion
```
`const` functions are already inlined, especially completely empty functions
so this does nothing
##########
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 => {
Review Comment:
I do think it is the same, if field.len() does not match it will go to the
`_ => {}` block, which is the same as not entering the if statement
##########
avro_test_helper/src/logger.rs:
##########
@@ -53,13 +53,47 @@ fn test_logger() -> &'static TestLogger {
})
}
+/// Clears all log messages stored in the thread-local storage.
+///
+/// # Panics
+/// This function will panic if:
+/// - The thread-local `LOG_MESSAGES` is already borrowed and cannot
+/// be mutably borrowed.
+///
+/// The panic includes a debug representation of the error encountered.
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:?}"),
});
}
+/// Asserts that a specific log message has not been logged.
+///
+/// This function checks the most recently logged message from a thread-local
+/// storage and ensures that it does not match the provided
`unexpected_message`.
+/// If the message does match, the function will panic with an appropriate
error
+/// message indicating the unexpected log entry.
+///
+/// # Arguments
+/// - `unexpected_message`: A string slice that represents the log message
+/// that is not expected to be logged. If this message matches the last
+/// logged message, the function will panic.
+///
+/// # Panics
+/// - This function will panic if the `unexpected_message` matches the most
+/// recently logged message.
+///
+/// # Example
+/// ```ignore
+/// // Assume LOG_MESSAGES is set up to capture log messages.
+/// log_message("Unexpected Error");
+/// assert_not_logged("Unexpected Error");
+/// // This will panic with the message:
+/// // "The following log message should not have been logged: 'Unexpected
Error'"
+///
+/// assert_not_logged("Non-existent Message");
+/// // This will pass without issue since the message was not logged.
#[track_caller]
pub fn assert_not_logged(unexpected_message: &str) {
LOG_MESSAGES.with(|msgs| match msgs.borrow().last() {
Review Comment:
```suggestion
LOG_MESSAGES.with_borrow(|msgs| match msgs.last() {
```
##########
avro_test_helper/src/logger.rs:
##########
@@ -70,6 +104,43 @@ pub fn assert_not_logged(unexpected_message: &str) {
});
}
+/// Asserts that the specified log message has been logged and removes it from
+/// the stored log messages.
+///
+/// # Parameters
+/// - `expected_message`: A string slice that holds the log message to be
asserted.
+///
+/// # Panics
+/// This function will panic if the `expected_message` has not been logged. The
+/// panic message will indicate the missing log message.
+///
+/// # Behavior
+/// - The function verifies if the `expected_message` exists in the log
messages
+/// stored using the thread-local storage (`LOG_MESSAGES`).
+/// - If the message is found, it is removed from the log messages and the
function
+/// completes successfully.
+/// - If the message is not found, a panic is triggered with an explanatory
message.
+///
+/// # Usage
+/// This function is typically used in unit tests or scenarios where it is
important
+/// to ensure specific messages are logged during execution.
+///
+/// # Example
+/// ```ignore
+/// // Assuming LOG_MESSAGES is set up and some log messages have been
recorded:
+/// assert_logged("Expected log entry");
+/// ```
+///
+/// # Notes
+/// - The function uses the `#[track_caller]` attribute to improve debugging by
+/// providing more accurate information about the location of the panic in
the
+/// source code.
+/// - The `LOG_MESSAGES` must be a thread-local data structure that supports
+/// borrowing and mutating of a collection of string messages.
+///
+/// # Thread Safety
+/// This function relies on thread-local variables to manage logs for each
thread
+/// independently.
Review Comment:
```suggestion
/// 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
/// 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 panic as the first call removed the matching log messages
/// assert_logged("Something went wrong");
/// ```
```
##########
avro_test_helper/src/logger.rs:
##########
@@ -53,13 +53,47 @@ fn test_logger() -> &'static TestLogger {
})
}
+/// Clears all log messages stored in the thread-local storage.
+///
+/// # Panics
+/// This function will panic if:
+/// - The thread-local `LOG_MESSAGES` is already borrowed and cannot
+/// be mutably borrowed.
+///
+/// The panic includes a debug representation of the error encountered.
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:?}"),
});
}
+/// Asserts that a specific log message has not been logged.
+///
+/// This function checks the most recently logged message from a thread-local
+/// storage and ensures that it does not match the provided
`unexpected_message`.
+/// If the message does match, the function will panic with an appropriate
error
+/// message indicating the unexpected log entry.
+///
+/// # Arguments
+/// - `unexpected_message`: A string slice that represents the log message
+/// that is not expected to be logged. If this message matches the last
+/// logged message, the function will panic.
+///
+/// # Panics
+/// - This function will panic if the `unexpected_message` matches the most
+/// recently logged message.
+///
+/// # Example
+/// ```ignore
+/// // Assume LOG_MESSAGES is set up to capture log messages.
+/// log_message("Unexpected Error");
+/// assert_not_logged("Unexpected Error");
+/// // This will panic with the message:
+/// // "The following log message should not have been logged: 'Unexpected
Error'"
+///
+/// assert_not_logged("Non-existent Message");
+/// // This will pass without issue since the message was not logged.
Review Comment:
Shouldn't this check all logs instead of just the last log?
##########
avro_test_helper/src/logger.rs:
##########
Review Comment:
```suggestion
LOG_MESSAGES.with_borrow_mut(|msgs| {
msgs.retain(|msg| {
```
##########
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 => {
Review Comment:
But this file will be overwritten by #512, so it doesn't really matter
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
