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

Run clippy in ci #542

Closed
cospectrum opened this issue Dec 23, 2024 · 4 comments
Closed

Run clippy in ci #542

cospectrum opened this issue Dec 23, 2024 · 4 comments

Comments

@cospectrum
Copy link

The contribution guide suggests running clippy. There are several pull requests that use the cargo clippy fix. But at the same time, I did not find a ci job that would check clippy, which looks strange.

@travenin
Copy link
Contributor

Running clippy in CI for every PR would be very good idea to keep codebase in better shape. However, there are quite a lot of issues in the main branch currently. Majority of them should be fixed first.

However, I am interested in working on this issue.

@pereman2
Copy link
Collaborator

I've been avoiding this as much as possible because I hate some things from clippy, but overall it is a good direction. I would suggest using this command as the entrypoint cargo clippy -- -A clippy::all -W clippy::correctness -W clippy::perf -W clippy::suspicious --deny=warnings

@travenin
Copy link
Contributor

Good point. It's not necessary to keep clippy perfectly happy. Your suggested command looks good place to start. 👍

pereman2 added a commit that referenced this issue Dec 29, 2024
…Preston Thorpe

There is no semantic changes in this PR, the clippy command came from
@pereman2's suggestion in #542
There was more to fix than I previously thought. I originally set out to
refactor out some of the logic in `vdbe::step`, but with some actual
semantic changes. That file: `vdbe/mod.rs` is so full that it required
moving the `Insn` enum to another file, so I figured I would just put
some non-semantic changes all together so it's easier to review and get
that done first... and figured I'd fix some clippy warnings while I was
at it. Also adjusted the actions to `checkout/@v3`.
The project is obviously so early that there are going to be a decent
amount of things like unused fields or methods, which is why I was
originally not really pro clippy.. but seeing how many genuinely good
improvements it recommended, I think it's probably the right way to go.

Closes #563
@penberg
Copy link
Collaborator

penberg commented Dec 31, 2024

We run Clippy now as part of the CI as of f6cd707.

@penberg penberg closed this as completed Dec 31, 2024
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

No branches or pull requests

4 participants