Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discuss: Should we implement custom lints? #5644

Closed
HaoYang670 opened this issue Mar 20, 2023 · 4 comments
Closed

Discuss: Should we implement custom lints? #5644

HaoYang670 opened this issue Mar 20, 2023 · 4 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@HaoYang670
Copy link
Contributor

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 (#5640 (comment)). 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 #5640 (comment), and #4561 (comment)). 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 HaoYang670 added enhancement New feature or request help wanted Extra attention is needed labels Mar 20, 2023
@HaoYang670
Copy link
Contributor Author

cc @iajoiner @alamb.

@alamb
Copy link
Contributor

alamb commented Mar 20, 2023

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
Copy link
Contributor Author

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.

@alamb
Copy link
Contributor

alamb commented Feb 23, 2025

I think our current lints with clippy have served us well and we have used custom lints where appriate

Let's open new issues if we have specific ideas for new lints

@alamb alamb closed this as completed Feb 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants