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

add Formatter::debug #49068

Closed
wants to merge 3 commits into from
Closed

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Mar 16, 2018

Adds (to std::fmt::Formatter):

    fn debug<D: Debug>(&mut self, d: D) -> Result {
        <D as Debug>::fmt(&d, self)
    }

which is intended to aid in Debug impls and read better than D::fmt(&d, fmt).

@Centril Centril added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Mar 16, 2018
@Centril Centril requested a review from sfackler March 16, 2018 00:28
@rust-highfive
Copy link
Collaborator

r? @kennytm

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 16, 2018
@Centril
Copy link
Contributor Author

Centril commented Mar 16, 2018

r? @sfackler

@rust-highfive rust-highfive assigned sfackler and unassigned kennytm Mar 16, 2018
@kennytm
Copy link
Member

kennytm commented Mar 16, 2018

In the example, why not simply use write!(formatter, "{:?} => {:?}", self.0, self.1)...?

@Centril
Copy link
Contributor Author

Centril commented Mar 16, 2018

Per discussion with @kennytm on #rust-libs I updated the example to one where you can't use the above snippet (since there's a diff btw {:?} and {:#?}).

@ollie27
Copy link
Member

ollie27 commented Mar 16, 2018

Why only for Debug? Why is it taking it's argument by value when it only uses a reference?

@sfackler
Copy link
Member

If we had done the fmt traits "right" their method names would match the type names rather than all being fmt so this wouldn't be a problem. How about adding those methods to the fmt traits instead?

pub trait Display {
    fn fmt(&self, fmt: &mut fmt::Formatter) -> fmt::Result;

    fn display(&self, fmt: &mut fmt::Formatter) -> fmt::Result {
        self.fmt(fmt)
    }
}

@kennytm
Copy link
Member

kennytm commented Mar 17, 2018

@sfackler This will cause new methods named .display()/.debug() etc appear on types impl/deriveing the format traits, which may not be a good idea (even if they aren't visible when not useing the trait).

@sfackler
Copy link
Member

Adding new methods to a trait will cause new methods to appear, yeah. This would not be the first time we've added methods to a trait. We'd want to run crater to see if there's a surprising amount of breakage like we did for e.g. Ord::min.

@kennytm
Copy link
Member

kennytm commented Mar 17, 2018

Sure, but these are not like super useful method that would be commonly used, so I prefer not to take the risk here :D

@Centril
Copy link
Contributor Author

Centril commented Mar 18, 2018

Hmm; while I agree on @sfackler's point that .debug(..) rather than .fmt(..) would have been appropriate, that was not the main goal... The main goal for me was that you often use fmt.xyz() and then all of a sudden the polarity is reversed, which reads poorly. So fmt.foo() uniformly reads better.
Obviously this is not an important addition, but kinda nice?

@ollie27 I can change to include & if we think that's best.

@bors
Copy link
Contributor

bors commented Mar 19, 2018

☔ The latest upstream changes (presumably #48978) made this pull request unmergeable. Please resolve the merge conflicts.

@shepmaster
Copy link
Member

It's unclear to me where this PR stands. @Centril, do you believe you've addressed all the feedback from @sfackler?

@Centril
Copy link
Contributor Author

Centril commented Mar 23, 2018

@Centril, do you believe you've addressed all the feedback from @sfackler?

That's not for me to decide; but I think @sfackler suggested addition tries to solve a different issue, that .fmt(..) on Debug and Display conflict. But inside a Debug implementation, the addition of fmt.debug(..) is intended mainly for decreasing asymmetry of reversing the positions of fmt and the thing being formatted. Of course, we could decide that the asymmetry is not a problem, and so close this PR -- which I'm entirely fine with. I leave it up to you to decide whether this is a problem that needs solving =)

@sfackler
Copy link
Member

The asymmetry is with debug_struct, etc? I don't really see that as an issue honestly. It seems pretty natural to want to delegate the call to one of your fields like you would hash or whatever.

@Centril
Copy link
Contributor Author

Centril commented Mar 28, 2018

@sfackler

The asymmetry is with debug_struct, etc?

Indeed it is.

I don't really see that as an issue honestly.

Personally, it bugs me a bit, but not much. Shall I close the PR then?

@sfackler
Copy link
Member

Yeah I'd lean towards closing.

@Centril
Copy link
Contributor Author

Centril commented Mar 29, 2018

Alright, closing then =)

@Centril Centril closed this Mar 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants