Skip to content

Conversation

@kurtjd
Copy link
Contributor

@kurtjd kurtjd commented Jun 15, 2025

Running cargo clippy --tests revealed several issues. This PR addresses those issues.

Of note is that the style lint was changed to deny. This is because clippy internally overrides a false positive of needless_return which conflicts with forbid. The generated error message notes this will soon become a hard error and links to this issue recommending replacing forbid with deny.

@kurtjd kurtjd requested a review from a team as a code owner June 15, 2025 17:27
@kurtjd kurtjd requested a review from felipebalbi June 15, 2025 17:27
@kurtjd kurtjd self-assigned this Jun 15, 2025
@kurtjd kurtjd moved this to In review in Embedded Controller Jun 15, 2025
felipebalbi
felipebalbi previously approved these changes Jun 15, 2025
@jerrysxie
Copy link
Contributor

Running cargo clippy --tests revealed several issues. This PR addresses those issues.

Of note is that the style lint was changed to deny. This is because clippy internally overrides a false positive of needless_return which conflicts with forbid. The generated error message notes this will soon become a hard error and links to this issue recommending replacing forbid with deny.

Should we consider adding a workflow to run cargo clippy --test as well to make sure that this does not reoccur in the future?

@kurtjd
Copy link
Contributor Author

kurtjd commented Jun 19, 2025

Running cargo clippy --tests revealed several issues. This PR addresses those issues.
Of note is that the style lint was changed to deny. This is because clippy internally overrides a false positive of needless_return which conflicts with forbid. The generated error message notes this will soon become a hard error and links to this issue recommending replacing forbid with deny.

Should we consider adding a workflow to run cargo clippy --test as well to make sure that this does not reoccur in the future?

I think that's a good idea. I'll see if I can add it to this commit.

@kurtjd kurtjd force-pushed the fix-clippy-tests branch 2 times, most recently from 7669ee4 to 9169641 Compare June 19, 2025 20:45
@kurtjd
Copy link
Contributor Author

kurtjd commented Jun 19, 2025

Running cargo clippy --tests revealed several issues. This PR addresses those issues.
Of note is that the style lint was changed to deny. This is because clippy internally overrides a false positive of needless_return which conflicts with forbid. The generated error message notes this will soon become a hard error and links to this issue recommending replacing forbid with deny.

Should we consider adding a workflow to run cargo clippy --test as well to make sure that this does not reoccur in the future?

I've updated the workflow to have clippy check all targets (including test) and all features. I intentionally kept one of the arguments as -F clippy::style (instead of -D clippy::style) to ensure a clippy warning would be generated. We can see it did indeed get caught by the workflow: https://github.com/OpenDevicePartnership/tmp108/actions/runs/15766043203/job/44442594830?pr=20#step:4:24

However, for some reason it's not counting as a failed check and raising any workflow issues. The same thing is happening on other ODP repos, whereas it works as expected on my personal forks. Might have to investigate if there are some repo settings causing this.

@kurtjd kurtjd force-pushed the fix-clippy-tests branch from 9169641 to ed1cc5d Compare June 19, 2025 20:52
@jerrysxie
Copy link
Contributor

Running cargo clippy --tests revealed several issues. This PR addresses those issues.
Of note is that the style lint was changed to deny. This is because clippy internally overrides a false positive of needless_return which conflicts with forbid. The generated error message notes this will soon become a hard error and links to this issue recommending replacing forbid with deny.

Should we consider adding a workflow to run cargo clippy --test as well to make sure that this does not reoccur in the future?

I've updated the workflow to have clippy check all targets (including test) and all features. I intentionally kept one of the arguments as -F clippy::style (instead of -D clippy::style) to ensure a clippy warning would be generated. We can see it did indeed get caught by the workflow: https://github.com/OpenDevicePartnership/tmp108/actions/runs/15766043203/job/44442594830?pr=20#step:4:24

However, for some reason it's not counting as a failed check and raising any workflow issues. The same thing is happening on other ODP repos, whereas it works as expected on my personal forks. Might have to investigate if there are some repo settings causing this.

It did show 2 workflow failures for me:
image

@kurtjd kurtjd force-pushed the fix-clippy-tests branch from ed1cc5d to b9c2c54 Compare June 19, 2025 20:56
@kurtjd
Copy link
Contributor Author

kurtjd commented Jun 19, 2025

Running cargo clippy --tests revealed several issues. This PR addresses those issues.
Of note is that the style lint was changed to deny. This is because clippy internally overrides a false positive of needless_return which conflicts with forbid. The generated error message notes this will soon become a hard error and links to this issue recommending replacing forbid with deny.

Should we consider adding a workflow to run cargo clippy --test as well to make sure that this does not reoccur in the future?

I've updated the workflow to have clippy check all targets (including test) and all features. I intentionally kept one of the arguments as -F clippy::style (instead of -D clippy::style) to ensure a clippy warning would be generated. We can see it did indeed get caught by the workflow: https://github.com/OpenDevicePartnership/tmp108/actions/runs/15766043203/job/44442594830?pr=20#step:4:24
However, for some reason it's not counting as a failed check and raising any workflow issues. The same thing is happening on other ODP repos, whereas it works as expected on my personal forks. Might have to investigate if there are some repo settings causing this.

It did show 2 workflow failures for me: image

Yes I just figured it out, need to add:

fail_on_error: true

jerrysxie
jerrysxie previously approved these changes Jun 19, 2025
@jerrysxie jerrysxie enabled auto-merge (squash) June 19, 2025 21:34
@jerrysxie jerrysxie requested review from Copilot and tullom July 1, 2025 17:17
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR resolves Clippy warnings in test modules, streamlines data initialization, and aligns lint configurations.

  • Swapped Vec allocations for fixed-size arrays in tests to satisfy Clippy.
  • Renamed ambiguous tmp variables to tmp108 and made default construction explicit via Configuration::default().
  • Updated style lint level to deny and enhanced the GitHub Actions Clippy workflow with --all-features, --all-targets, and fail_on_error.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
src/blocking.rs Converted test Vec to array, renamed tmp to tmp108, and replaced Default::default() with Configuration::default().
src/asynchronous.rs Applied the same array and renaming changes in async tests, moved Polarity/ThermostatMode imports under cfg(feature).
Cargo.toml Changed Clippy style lint from forbid to deny to avoid conflicts with future hard errors.
.github/workflows/check.yml Added --all-features, --all-targets, fail_on_error, and adjusted Clippy flags.
Comments suppressed due to low confidence (2)

.github/workflows/check.yml:75

  • The workflow still forbids clippy::style with -F but Cargo.toml now uses style = "deny". Change -F clippy::style to -D clippy::style to match the lint level.
          clippy_flags: --all-features --all-targets -- -Dwarnings -F clippy::suspicious -F clippy::correctness -D clippy::perf -F clippy::style

.github/workflows/check.yml:75

  • Clippy perf lints are set with -D but Cargo.toml uses perf = "forbid". Use -F clippy::perf to explicitly forbid performance lints rather than -D.
          clippy_flags: --all-features --all-targets -- -Dwarnings -F clippy::suspicious -F clippy::correctness -D clippy::perf -F clippy::style

@jerrysxie
Copy link
Contributor

@kurtjd it seems like this PR needs to be updated after #21 has been merged,

@kurtjd
Copy link
Contributor Author

kurtjd commented Jul 28, 2025

@kurtjd it seems like this PR needs to be updated after #21 has been merged,

Thanks for the heads up. Done.

@kurtjd kurtjd force-pushed the fix-clippy-tests branch from ff0ee31 to 2f92724 Compare July 28, 2025 20:45
@jerrysxie jerrysxie requested a review from a team July 31, 2025 12:56
@jerrysxie jerrysxie merged commit 2a40361 into OpenDevicePartnership:main Aug 4, 2025
11 checks passed
@github-project-automation github-project-automation bot moved this from In review to Done in Embedded Controller Aug 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants