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(clippy): apply needless_lifetimes clippy fix #848

Merged
merged 1 commit into from
Jan 23, 2025

Conversation

mkatychev
Copy link
Contributor

fix(clippy): apply needless_lifetimes clippy fix

Description

#845 CI run complained about unelided lifetimes, ran cargo clippy --fix to solve the issue.

Checklist

Checklist before merging:

  • CHANGELOG.md updated
  • README.md up-to-date

Copy link
Member

@Xophmeister Xophmeister left a comment

Choose a reason for hiding this comment

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

Has this linting rule been triggered due to the change in toolchain in #845?

I think it's a positive change, so I'm inclined to accept it for that reason, but if our current toolchain doesn't pick it up, we can't enforce it. That's only a problem insofar as this might lead to drift.

@yannham
Copy link
Member

yannham commented Jan 23, 2025

Has this linting rule been triggered due to the change in toolchain in #845?

Yes, there is a nix flake update there, which updates to a more recent version of the Rust toolchain, which usually comes with a more demanding clippy. If #845 is getting merged, the CI will warn in the future. Even if it's not yet enforced by the CI, it'll be one day or the other, because we want to keep the flake.lock up to date as much as possible, I suppose.

@Xophmeister
Copy link
Member

Xophmeister commented Jan 23, 2025

Thanks, @yannham 🙏

Because the nix flake update is preventing the CI from running correctly in #845, what I propose to do is:

  1. Accept and merge this PR.
  2. Create a new PR that does a nix flake update only, to upgrade our toolchains and, thus, get the CI working again for feat(query): Add OpenSCAD formatter #845. Hopefully this should be an easy fix and not lead to a litany of linting errors that will need to be corrected 🤞

If that all works out, then @mkatychev, I think you can ignore this comment I just made on #845...

@Xophmeister Xophmeister merged commit 118bb94 into tweag:main Jan 23, 2025
10 checks passed
@mkatychev
Copy link
Contributor Author

@Xophmeister appreciate the rundown, I hadn't thought of the flake updating the toolchain to be an issue, would version pinning a rust toolchain similar to rust-toolchain.toml in the flake be of use?

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.

3 participants