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

Set max line length to Rust's defaults #750

Merged
merged 1 commit into from
Mar 12, 2024

Conversation

aaronmondal
Copy link
Contributor

@aaronmondal aaronmondal commented Mar 11, 2024

... which is 100 🫠


This change is Reviewable

Copy link
Contributor Author

@aaronmondal aaronmondal left a comment

Choose a reason for hiding this comment

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

+@allada +@zbirenbaum +@MarcusSorealheis +@adam-singer

Inspired by a conversation in #748 noting that we sometimes have some hard-to-read long lines.

Reviewable status: 0 of 4 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Local / ubuntu-22.04, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, pre-commit-checks, publish-image, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, vale, windows-2022 / stable, zig-cc ubuntu-20.04, zig-cc ubuntu-22.04 (waiting on @adam-singer, @allada, @MarcusSorealheis, and @zbirenbaum)

Copy link
Contributor

@adam-singer adam-singer left a comment

Choose a reason for hiding this comment

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

Copy link
Contributor

@zbirenbaum zbirenbaum left a comment

Choose a reason for hiding this comment

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

I was under the impression that it's 100 as well

Reviewable status: 0 of 4 LGTMs obtained (waiting on @adam-singer, @allada, and @MarcusSorealheis)

Copy link
Contributor Author

@aaronmondal aaronmondal left a comment

Choose a reason for hiding this comment

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

Oh yeah looks like my information was outdated: rust-lang/rust-guidelines#12

I updated the commit message. The value is "set to default" by removing the max_width line in .rustfmt.toml, so this PR is already using the 100 line length limit.

Reviewable status: 0 of 4 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Vercel, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), pre-commit-checks, publish-image, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, vale, zig-cc ubuntu-20.04 (waiting on @adam-singer, @allada, @MarcusSorealheis, and @zbirenbaum)

Copy link
Contributor

@adam-singer adam-singer left a comment

Choose a reason for hiding this comment

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

I'm less invested in what the setting is and more of "we have a setting we like and is enforced by tooling", so with that reasoning will give a shipit. I do wonder if "now is the right time" to land this change, usually these kind of sweeping changes of formatting bork others reviews/rebase. If nothing big in flight and we are good with the setting, :lgtm:

Reviewed 93 of 93 files at r1, all commit messages.
Reviewable status: 1 of 4 LGTMs obtained (waiting on @allada, @MarcusSorealheis, and @zbirenbaum)

Copy link
Contributor Author

@aaronmondal aaronmondal left a comment

Choose a reason for hiding this comment

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

-@zbirenbaum -@allada -@MarcusSorealheis

Reviewable status: :shipit: complete! 1 of 1 LGTMs obtained

@aaronmondal aaronmondal merged commit a876cce into TraceMachina:main Mar 12, 2024
25 checks passed
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.

None yet

5 participants