Skip to content

Clippy fixes#303

Merged
gabotechs merged 2 commits intodatafusion-contrib:mainfrom
JSOD11:jsod/clippy-fixes-01-22-26
Jan 23, 2026
Merged

Clippy fixes#303
gabotechs merged 2 commits intodatafusion-contrib:mainfrom
JSOD11:jsod/clippy-fixes-01-22-26

Conversation

@JSOD11
Copy link
Collaborator

@JSOD11 JSOD11 commented Jan 22, 2026

In #256, we noticed that some clippy warnings were not caught by CI. I've taken a look, and I believe the relevant line is here:

cargo clippy --all-targets --features integration

If we want the CI to fail on a clippy warning like for this print statement, seems like we have to change it to

cargo clippy --all-targets --features integration -- -D warnings

@JSOD11 JSOD11 marked this pull request as ready for review January 22, 2026 14:11
with:
components: clippy
- run: cargo clippy --all-targets --features integration
- run: cargo clippy --all-targets --features integration -- -D warnings
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 I'm realizing something... I think this might not be getting applied to any of the tests behind the tpch, tpcds and clickbench features.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we need to say something like --all-features?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, from what I can tell clippy right now is only being applied to the integration feature.

Upstream, clippy in CI here looks like it runs this script, which ends up running

cargo clippy --all-targets --workspace --features avro,integration-tests,extended_tests -- -D warnings

so the -- -D warnings is definitely there, and it looks like they manually list out the features rather than using --all-features, presumably because there are so many features it could slow down CI. For us, I think --all-features could be a good idea. Thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 yeap! lets go with that

@gabotechs gabotechs merged commit 2473ac3 into datafusion-contrib:main Jan 23, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants