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

Fix cargo clippy warnings #383

Closed
kate-goldenring opened this issue Sep 22, 2021 · 5 comments · Fixed by #442
Closed

Fix cargo clippy warnings #383

kate-goldenring opened this issue Sep 22, 2021 · 5 comments · Fixed by #442
Assignees
Labels

Comments

@kate-goldenring
Copy link
Contributor

kate-goldenring commented Sep 22, 2021

cargo clippy is a linter that suggest Rust code improvements. We currently check all PRs for clippy errors before merging in our Check Rust workflow; however, this does not seem to prevent merging code that adds more clippy warnings.

All clippy warnings should be addressed:
cargo clippy --all

Furthermore, clippy errors in tests should be addressed
cargo clippy --all-targets --all-features -- -D warnings

Some of these may be able to be addressed via cargo clippy --fix

Also, our Check Rust workflow could be updated to run cargo clippy --all-targets --all-features -- -D warnings instead of cargo clippy --all so as to always check test code and disallow merging code that creates clippy warnings

@kate-goldenring kate-goldenring added bug Something isn't working good first issue Good for newcomers and removed bug Something isn't working labels Sep 22, 2021
@Saran51
Copy link

Saran51 commented Oct 1, 2021

I will work on this as part of OSD contribution.

@kate-goldenring
Copy link
Contributor Author

kate-goldenring commented Oct 1, 2021

Addressing the function has too many arguments warnings may require creating new structures in the code and could be skipped. If this is something we dont think needs to be fixed, we could create a clippy.toml file and add a configuration line to ignore too many arguments errors.

Example error:

warning: this function has too many arguments (10/7)
   --> controller/src/util/pod_watcher.rs:478:5
    |
478 | /     async fn create_or_update_service(
479 | |         &self,
480 | |         instance_name: &str,
481 | |         configuration_name: &str,
...   |
488 | |         kube_interface: &impl KubeInterface,
489 | |     ) -> anyhow::Result<()> {

@Saran51 Saran51 mentioned this issue Oct 1, 2021
8 tasks
@kate-goldenring kate-goldenring added this to Triage needed in Akri Roadmap Oct 5, 2021
@kate-goldenring kate-goldenring moved this from Triage needed to In progress in Akri Roadmap Oct 5, 2021
@github-actions
Copy link
Contributor

Issue has been automatically marked as stale due to inactivity for 90 days. Update the issue to remove label, otherwise it will be automatically closed.

@kate-goldenring
Copy link
Contributor Author

@Saran51 any updates on your progress with this?

@Ragnyll
Copy link
Contributor

Ragnyll commented Jan 23, 2022

Hey, its been a few months since there's been any commit by the assignee on this issue. I've raised a PR that answers this issue and is up to date. If that is inappropriate feel free to close, but I think it should help. #442

Akri Roadmap automation moved this from In progress to Done Jan 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants