Skip to content

code quality (linting and formatting) #303

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

Closed
19 tasks done
danieleades opened this issue Nov 11, 2022 · 5 comments
Closed
19 tasks done

code quality (linting and formatting) #303

danieleades opened this issue Nov 11, 2022 · 5 comments

Comments

@danieleades
Copy link
Contributor

danieleades commented Nov 11, 2022

It doesn't appear that linting or formatting are applied/checked in this repo. Would you be interested in pull requests for these?

I can split these into separate issues if you like. I think the value of auto-formatting is well documented, particularly for open-source projects.

clippy also is useful for catching both correctness and style issues. The following PRs are mostly fixes for various clippy lints

see:

note that some of these may need to be rebased after others are merged (if they are merged, of course).

#305 in particular has potential to be a little hairy. that should probably go last, and also has the potential to cause headaches for other open PRs. My advice, rip the bandaid off. It'll save a lot of headaches in future.

@danieleades
Copy link
Contributor Author

you can probably guess from the PRs that i'm gearing up to adding a linting job to CI, but first I need to make sure said job would actually pass

@dwrensha
Copy link
Member

Autoformatting sounds good to me. I'm a bit more skeptical about Clippy, as it seems to make bad suggestions sometimes (e.g. https://github.com/capnproto/capnproto-rust/pull/312/files#r1020222229 ). I suppose we could add specific suppressions when we disagree with Clippy?

@danieleades
Copy link
Contributor Author

suppose we could add specific suppressions when we disagree with Clippy?

exactly! better to opt out of specific lints than not have any linting, i think. I'll close the PR you reference, and when I open a PR to add automatic linting i'll suppress those warnings.

@danieleades
Copy link
Contributor Author

getting very close now. Just a handful more lint errors and warnings to address and then linting can be added to CI

@dwrensha
Copy link
Member

Closing, because all of the checkmarks are filled in. Thanks!

@dwrensha dwrensha mentioned this issue Jan 13, 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

2 participants