Change Debug impl of SocketAddr and IpAddr to match their Display output #74409
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
I certainly agree that there isn't much value in the extra newlines when pretty-printing. As for changing the Debug impl to match the Display, I know it can change at any time, but we might still want to do a quick crater run just to make sure it doesn't break anyone. |
|
A crater run sounds fine to me! |
|
@bors try |
|
⌛ Trying commit 805b171bcab72bdfd11f18d82b3c029e0d40d73d with merge 0abbfa7f48aec40f12e368b7ff814b60d1e4d145... |
|
💥 Test timed out |
|
@bors retry |
|
⌛ Trying commit 805b171bcab72bdfd11f18d82b3c029e0d40d73d with merge 03a1ea71b075ab964b5278bc6e74cd6c52c36ee0... |
|
☀️ Try build successful - checks-actions, checks-azure |
|
@craterbot run |
|
👌 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
craterbot
added
S-waiting-on-crater
|
Given that that crater queue is pretty long right now (with probably more important PRs), I thought about this again. While I still don't mind a crater run, I don't really think it is necessary here. For one, we explicitly documented that the output is not stable, so even if some crates fail I would tend to say "that's their fault". And also, I really can't imagine people using the debug output in any way that would be caught by running the tests. I can imagine people using the So yeah, maybe we should remove this PR from the crater queue again? |
|
@LukasKalbertodt I can easily imagine a crate that compares the overall output of a test, where that test output includes various values such as an |
|
FWIW historically we've not done much about cases where people are comparing or otherwise making use of Debug output in cases like this, though we can of course be more stringent in this case. We could try and catch this in 1.47 beta crater as well (so in ~6 weeks, roughly, once this hits beta). I'd personally be fine with trying to catch this in beta crater or even just letting it roll out, Debug impls are IMO sort of outside stability guarantees, and doubly so when we have a Display impl on the same type. |
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
|
craterbot
added
S-waiting-on-review
|
I checked all regressions and it looks like 3 are caused by this PR:
Those indeed compare the I think this kind of breakage is absolutely acceptable, especially since the instability of the |
|
@LukasKalbertodt I think this is acceptable as well. No huge swathes of dependencies (two of them are from the same project), and no surprises. I think it'd be reasonable to proceed with this. |
|
@bors r+ |
|
📌 Commit 805b171bcab72bdfd11f18d82b3c029e0d40d73d has been approved by |
bors
added
S-waiting-on-bors
|
☔ The latest upstream changes (presumably #73265) made this pull request unmergeable. Please resolve the merge conflicts. |
bors
added
S-waiting-on-author
This has already been done for `SocketAddrV4`, `SocketAddrV6`,
`IpAddrV4` and `IpAddrV6`. I don't see a point to keep the rather bad
to read derived impl, especially when pretty printing:
V4(
127.0.0.1
)
From the `Display`, one can easily and unambiguously see if it's V4 or
V6. Using `Display` as `Debug` is very convenient for configuration
structs (e.g. for webservers) that often just have a `derive(Debug)`
and are printed that way to the user.
LukasKalbertodt
force-pushed
the
improve-debug-impl-of-socketaddr-ipaddr
branch
from
805b171 to
6293dca
Compare
|
rebased |
Muirrum
added
S-waiting-on-author
Dylan-DPC-zz
added
S-waiting-on-review
|
@bors r=Amanieu |
|
📌 Commit 6293dca has been approved by |
bors
added
S-waiting-on-bors
Rollup of 17 pull requests Successful merges: - rust-lang#73943 (Document the unsafe keyword) - rust-lang#74062 (deny(unsafe_op_in_unsafe_fn) in libstd/ffi/c_str.rs) - rust-lang#74185 (Remove liballoc unneeded explicit link) - rust-lang#74192 (Improve documentation on process::Child.std* fields) - rust-lang#74409 (Change Debug impl of SocketAddr and IpAddr to match their Display output) - rust-lang#75195 (BTreeMap: purge innocent use of into_kv_mut) - rust-lang#75214 (Use intra-doc links in `mem::manually_drop` & `mem::maybe_uninit`) - rust-lang#75432 (Switch to intra-doc links in `std::process`) - rust-lang#75482 (Clean up E0752 explanation) - rust-lang#75501 (Move to intra doc links in std::ffi) - rust-lang#75509 (Tweak suggestion for `this` -> `self`) - rust-lang#75511 (Do not emit E0228 when it is implied by E0106) - rust-lang#75515 (Bump std's libc version to 0.2.74) - rust-lang#75517 (Promotion and const interning comments) - rust-lang#75519 (BTreeMap: refactor splitpoint and move testing over to unit test) - rust-lang#75530 (Switch to intra-doc links in os/raw/*.md) - rust-lang#75531 (Migrate unit tests of btree collections to their native breeding ground) Failed merges: r? @ghost
This has already been done for
SocketAddrV4,SocketAddrV6,IpAddrV4andIpAddrV6. I don't see a point to keep the rather bad to read derived impl, especially so when pretty printing:From the
Display, one can easily and unambiguously see if it's V4 or V6. Two examples:Luckily the docs explicitly state that
Debugoutput is not stable and that it may be changed at any time.Using
DisplayasDebugis very convenient for configuration structs (e.g. for webservers) that often just have aderive(Debug)and are printed that way to the one starting the server.