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

Enable pedantic clippy on all crates and deal with the fallout #1658

Open
davidv1992 opened this issue Sep 19, 2024 · 7 comments
Open

Enable pedantic clippy on all crates and deal with the fallout #1658

davidv1992 opened this issue Sep 19, 2024 · 7 comments
Labels
good first issue Good for newcomers

Comments

@davidv1992
Copy link
Member

davidv1992 commented Sep 19, 2024

We really should do as it would have prevented a bug. But this is going to take some effort... Be prepared that this is not the most fun task.

Also, be carefull not to introduce msrv changes with this.

@davidv1992 davidv1992 added the good first issue Good for newcomers label Sep 19, 2024
@van-sprundel
Copy link

I would like to work on this, but I was wondering which bug could've been prevented with the pedantic lint group.

@davidv1992
Copy link
Member Author

We had a bug in our time jump handling in which we did not properly correct the filter state for the jump, due to ignoring the return value of a function. This is the specific example we have right now. The pedantic lints would have caught that particular pattern (as the function was a pure function).

@van-sprundel van-sprundel mentioned this issue Oct 12, 2024
3 tasks
@van-sprundel
Copy link

I'm not sure how to keep the impact minimal. The big one is ntp-proto, which has ~1000 loc changed. Do we just accept that this is going to conflict with other PRs?

@davidv1992
Copy link
Member Author

O wow, that is quite some impact, even more than I would have expected. Given that I do expect all of these to be small changes individually, I think accepting that it is going to give some headaches is the best we can do.

Should any larger restructurings be necessary in places, please do split of those into separate PRs if possible. Then we can deal with those before the large but mostly simple pr.

@van-sprundel
Copy link

van-sprundel commented Oct 16, 2024

What I can do is split the docs changes into a separate PR, then split it again for each crate. That should make it nicer to review at least

@davidv1992
Copy link
Member Author

Please wait one moment still. The doc changes aren't the problem, but I will post a review once I am done with it that does ask for a few splitups/changes

@davidv1992
Copy link
Member Author

Added review.

Note to self: we need to keep this issue open on merging that, as there is still quite some work to do cleaning up the allow marked areas. Need to reconsider then how beginner friendly that work is though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants