apache / datafusion

Apache DataFusion SQL Query Engine
https://datafusion.apache.org/
Apache License 2.0
6.29k stars 1.19k forks source link

Discuss: Should we implement custom lints? #5644

Open HaoYang670 opened 1 year ago

HaoYang670 commented 1 year ago

Is your feature request related to a problem or challenge? Please describe what you are trying to do.

While working on #5640, we find that Datafusion provides many helpers to make the code simpler (https://github.com/apache/arrow-datafusion/pull/5640#discussion_r1141546182). It is better for contributors to use them, instead of implement them own version or repeat themselves.

Currently, we find the room of improvement when reviewing the PR (for example https://github.com/apache/arrow-datafusion/pull/5640#discussion_r1141433380, and https://github.com/apache/arrow-datafusion/pull/4561#discussion_r1044408770). It is ideal that we can implement lints for them so that the CI/CD can find them automatically.

Describe the solution you'd like Implement custom lints for the project. It is ideal if we can implement a plugin for cargo clippy.

Describe alternatives you've considered We could not do this if it is not cost-effective.

Additional context A discussion 2 years ago: https://users.rust-lang.org/t/custom-lints-for-my-crates/54897

HaoYang670 commented 1 year ago

cc @iajoiner @alamb.

alamb commented 1 year ago

I worry if we require a non standard rust tool (like some clippy plugin) that would make it harder for people to contribute to datafusion as it would raise the barrier to entry

Tuning the existing standard tools to make the codebase more consistent, is a great idea, in my opinion

For example, we might consider enabling more clippy lints: https://github.com/influxdata/influxdb_iox/blob/c0a53ae5a99c12dd50c9eb2772afefc8430d622c/querier/src/lib.rs#L2-L11

#![warn(
    missing_copy_implementations,
    missing_docs,
    clippy::explicit_iter_loop,
    clippy::future_not_send,
    clippy::use_self,
    clippy::clone_on_ref_ptr,
    clippy::todo,
    clippy::dbg_macro
)]
HaoYang670 commented 1 year ago

Here is a blog: https://blog.trailofbits.com/2021/11/09/write-rust-lints-without-forking-clippy/ which introduced a crate for addling personal lints. It doesn't depend on cargo clippy, so that it won't affect the standard rust tool.