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

chore: Fix new clippy lints #9114

Merged
merged 5 commits into from
Jan 14, 2025
Merged

chore: Fix new clippy lints #9114

merged 5 commits into from
Jan 14, 2025

Conversation

upbqdn
Copy link
Member

@upbqdn upbqdn commented Jan 13, 2025

Motivation

Clippy has some new lints that make the CI fail.

Solution

  • Refactor the code so it passes the lint checks.

Tests

  • No new tests.

PR Author's Checklist

  • The PR name will make sense to users.
  • The PR provides a CHANGELOG summary.
  • The solution is tested.
  • The documentation is up to date.
  • The PR has a priority label.

PR Reviewer's Checklist

  • The PR Author's checklist is complete.
  • The PR resolves the issue.

@upbqdn upbqdn added A-rust Area: Updates to Rust code C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG P-High 🔥 labels Jan 13, 2025
@upbqdn upbqdn self-assigned this Jan 13, 2025
@upbqdn upbqdn requested review from a team as code owners January 13, 2025 16:38
@upbqdn upbqdn requested review from oxarbitrage and removed request for a team January 13, 2025 16:38
arya2
arya2 previously approved these changes Jan 14, 2025
@arya2
Copy link
Contributor

arya2 commented Jan 14, 2025

The release job is still failing, we could replace it with a patch job in this PR, or, @gustavovalverde could we remove it from the list of required jobs?

@gustavovalverde
Copy link
Member

@gustavovalverde could we remove it from the list of required jobs?

@arya2 Do we expect to fix the issue causing the job to fail? We can remove it a as required job, but it will keep creating red crosses across the repo and in the main branch, and I'd prefer to avoid that if possible. Another question is, do we need this to run in all PRs or just on the release PR?

@arya2
Copy link
Contributor

arya2 commented Jan 14, 2025

@arya2 Do we expect to fix the issue causing the job to fail?

Yes, but not for a while, I'm hoping --no-verify's behaviour will be restored in the next version of Rust, but failing that I think we have to set up a private registry or use another complicated workaround.

We can remove it a as required job, but it will keep creating red crosses across the repo and in the main branch, and I'd prefer to avoid that if possible.

Yeah, it might be better to just remove the job altogether, or replace it with a patch job for now.

Another question is, do we need this to run in all PRs or just on the release PR?

This one runs on all PRs to check that Zebra could still be released, though it's never fully worked because verifying that a crate can be published relies on publishing its dependencies, and we don't want to do that for every change (at least not to crates.io).

@upbqdn
Copy link
Member Author

upbqdn commented Jan 14, 2025

@gustavovalverde, @arya2, I opened #9119 atop this PR.

@upbqdn upbqdn requested a review from a team as a code owner January 14, 2025 19:25
@arya2
Copy link
Contributor

arya2 commented Jan 14, 2025

Needs a quick merge conflict resolution.

@gustavovalverde it may still need to be removed from the list of required jobs since the patch job was removed.

@upbqdn upbqdn requested a review from a team as a code owner January 14, 2025 19:31
@upbqdn upbqdn requested review from arya2 and removed request for a team January 14, 2025 19:31
Copy link
Contributor

@arya2 arya2 left a comment

Choose a reason for hiding this comment

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

Thank you!

@upbqdn
Copy link
Member Author

upbqdn commented Jan 14, 2025

I'm now thinking I should've opened #9119 against main since it would have passed CI without the lint updates in this PR, and we'd get a better merge history. I'll open it again against main.

@mergify mergify bot merged commit 410cac0 into main Jan 14, 2025
202 checks passed
@mergify mergify bot deleted the fix-clippy-lints branch January 14, 2025 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rust Area: Updates to Rust code C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG P-High 🔥
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants