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(*): Run rustfmt 1.7.0 and clippy 0.1.76 #4907

Closed
wants to merge 3 commits into from

Conversation

umgefahren
Copy link
Contributor

@umgefahren umgefahren commented Nov 20, 2023

Description

Ran cargo fmt and cargo clippy --fix with the respective new nightly versions.

Notes & open questions

While I have no issues with the fixes introduced by clippy, I have issues with the new rustfmt version.

As you can see the changes are quite severe and the change in style is stronger then in previous releases.

We should be careful when merging into master, because every PR that doesn't use the nightly compiler and runs the format will probably have a merge conflict. But we probably have to do it eventually.

An upside of the changes, at least from my POV is that changes are now spread over more lines, which could be beneficial for merge conflict resolution.

I would suggest we keep this PR open and I update it regularly with the nightly versions of rustfmt. It's a lot of work to merge that probably, so I'd suggest we merge the version that will be released eventually.

Change checklist

  • I have performed a self-review of my own code

@umgefahren
Copy link
Contributor Author

This single PR is so massive, it would probably be my biggest contribution (in the stats) by far, which is... interesting.

@thomaseizinger
Copy link
Contributor

I have issues with the new rustfmt version.

Me too and I am somewhat surprised that the Rust team would ship that!

While I have no issues with the fixes introduced by clippy

Can you extract that into a separate PR (and bump the nightly clippy version in CI). We will update to clippy once 1.75 is stable so we might as well fix those lints early.

Copy link
Contributor

mergify bot commented Nov 21, 2023

This pull request has merge conflicts. Could you please resolve them @umgefahren? 🙏

@umgefahren
Copy link
Contributor Author

I have issues with the new rustfmt version.

Me too and I am somewhat surprised that the Rust team would ship that!

Should we open an issue with them, if there isn't one already?

While I have no issues with the fixes introduced by clippy

Can you extract that into a separate PR (and bump the nightly clippy version in CI). We will update to clippy once 1.75 is stable so we might as well fix those lints early.

Will do, some of the remaining lints need manual fixing though and will introduce breaking changes in public APIs.

@thomaseizinger
Copy link
Contributor

I have issues with the new rustfmt version.

Me too and I am somewhat surprised that the Rust team would ship that!

Should we open an issue with them, if there isn't one already?

I think it may come as part of the next edition which would allow for such changes to be made AFAIK.

I have issues with the new rustfmt version.

Me too and I am somewhat surprised that the Rust team would ship that!

Should we open an issue with them, if there isn't one already?

While I have no issues with the fixes introduced by clippy

Can you extract that into a separate PR (and bump the nightly clippy version in CI). We will update to clippy once 1.75 is stable so we might as well fix those lints early.

Will do, some of the remaining lints need manual fixing though and will introduce breaking changes in public APIs.

Happy to merge the easy ones first and hold off on all the breaking ones until we do another release.

Copy link
Contributor

mergify bot commented Apr 15, 2024

This pull request has merge conflicts. Could you please resolve them @umgefahren? 🙏

@jxs
Copy link
Member

jxs commented Jun 13, 2024

closing as stale

@jxs jxs closed this Jun 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

Successfully merging this pull request may close these issues.

3 participants