carlosahs commented on issue #18881: URL: https://github.com/apache/datafusion/issues/18881#issuecomment-3650150750
@kumarUjjawal, let's remember that the purpose of `expect` is to be identical to `allow`, but [will cause a lint to be emitted when the code it decorates does not raise a lint warning](https://rust-lang.github.io/rfcs/2383-lint-reasons.html#expect-lint-attribute). With this in mind I want to highlight the following: + `datafusion-proto`: We should remove the `#[allow(deprecated)]` lints as `max_statistics_size` no longer exists ([ref](https://github.com/apache/datafusion/commit/fa1f8c192dd531d3f2fca61885eafa3e9002f0dd#diff-2f697c310af1a48521829d8bd665cf64b6415fbf88edd370efa30b1ed686354d)). `expect` is essentially helping us to identify that we don't need to add the `deprecated` attribute because there are no more deprecated fields for `ParquetOptions`: https://github.com/apache/datafusion/blob/fedddbcae49937ecc76bd5f2bf6acd00e9d15067/datafusion/common/src/config.rs#L656-L840. Timeline of `max_statistics_size`: 1. https://github.com/apache/datafusion/pull/14188. 2. https://github.com/apache/datafusion/pull/16690. + `datafusion-pruning`: For this one I think we need to first figure out if `this is only used by `parquet` feature right now` claims are still valid. `pruning_predicate.rs` was created from logic initially living in `lib.rs` as part of this refactor: https://github.com/apache/datafusion/pull/16587. And that PR was from Jun 27. It's likely claim is no longer valid since `expect` is failing with unfulfilled lint expectation errors, in which case I think we should remove the `#![allow(dead_code)]` + `datafusion-session`: I think it's best to enable `#![deny(clippy::allow_attributes)]` for future use cases. Case from `datafusion-proto` suggests if `expect` was used instead of `allow`, then `deprecated` attributes would've been removed too when `max_statistics_size` was removed. My suggestion when facing ```-D unfulfilled-lint-expectations``` errors is to first identify possible root causes, then track down the root cause, and finally document why an `allow` needs to be removed based on such findings. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
