-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
internal: Fix VfsPath Display and Debug impls #15097
Conversation
@bors r+ |
☀️ Test successful - checks-actions |
@@ -292,7 +292,7 @@ impl From<AbsPathBuf> for VfsPath { | |||
impl fmt::Display for VfsPath { | |||
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | |||
match &self.0 { | |||
VfsPathRepr::PathBuf(it) => fmt::Debug::fmt(&it, f), | |||
VfsPathRepr::PathBuf(it) => fmt::Display::fmt(&it, f), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could also replace all of these (including the ones in the Debug
impl below) with it.fmt(f)
. Method resolution will prefer the right one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’d rather not. It’s is true that
Method resolution will prefer the right one
but this also is one of the least obvious rules in Rust.
no strong feelings here though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe not! I think my main worry is that Debug&Disolay have a method with identical signature, which can be asily confused during method resolution.
However, given that we never import the traits directly, and only use them through fmt:: prefix, using .fmt should be OK.
internal: Simplify `VfsPath` `fmt` impls Fixes #15097 (comment)
fix #15087 (comment)
r? @matklad